1901666 - Move the handling of FxA notifications out of BrowserGlue.sys.mjs

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Sammy Khamis [:skhamis]]

In https://phabricator.services.mozilla.com/D212500#inline-1178898

From an architecture point of view it's kind of odd that this bit lives here but the actual notifications live in BrowserGlue.

:gijs brings up a good point that the actual handling of the notifications have been living in BrowserGlue.sys.mjs for some time (in addition to the soon-to-be-added CloseTab notifications). While there might've been some historical reason for this it seems there are enough FxA command notifications logic that we can probably pull out a decent chunk of them from BrowserGlue to closer to FxA.

Proposal:

1. Make a new class in services/fxaccounts that handles FxA Notifications (something like, FxANotificationHandler?)
2. Pull out all the logic for processing/showing/closing FxA tabs/notifications
3. Keep the observers in BrowserGlue and simplify the calls so it just passes the relevant data to the new class

Comment 1

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

When I was looking at this, I feel another reasonable option would be to move all of the sync/fxa stuff from BrowserGlue.sys.mjs to, say, services/sync/AccountsBrowserGlue.sys.mjs (not an ideal location but it would be next to UIState.sys.mjs) - BrowserGlue.sys.mjs would obviously need to arrange for this module to be imported as it is initialized, but it would clean things up quite a bit - moving many hundreds of lines from that 6k line file seems like it would be a win all around.

Gijs, what do you think? Is that too ambitious or a bad idea for other reasons?

Comment 2

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #1)

When I was looking at this, I feel another reasonable option would be to move all of the sync/fxa stuff from BrowserGlue.sys.mjs to, say, services/sync/AccountsBrowserGlue.sys.mjs (not an ideal location but it would be next to UIState.sys.mjs) - BrowserGlue.sys.mjs would obviously need to arrange for this module to be imported as it is initialized, but it would clean things up quite a bit - moving many hundreds of lines from that 6k line file seems like it would be a win all around.

Gijs, what do you think? Is that too ambitious or a bad idea for other reasons?

I think that's a fine idea - I had commented specifically on the notifications because there's an FxA module with 2 public properties that are only ever read and written from the BrowserGlue code, which I felt was a... peculiar... division of labour. But no objection at all to move even more things over to something more specialized / have BrowserGlue be less of a dumping ground.

In terms of the integration point for initialization, in the immediate term that could be a direct call from BrowserGlue (ie in the startup idle tasks or whatever we currently do); we may move to something more dependency-injection-style like https://phabricator.services.mozilla.com/D210432 later.

Comment 3

[Sammy Khamis [:skhamis]]

Attachment: Bug 1901666: Move Account-related code into its own BrowserGlue r=markh,gijs

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc411b5665be
Move Account-related code into its own BrowserGlue r=markh,Gijs,sync-reviewers,firefox-desktop-core-reviewers ,mossop

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/fc411b5665be