1079198 - Refactor MozLoopService.jsm

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P4)

Description

[Mike de Boer [:mikedeboer]]

It's starting to look like a monstrous beast with a scary head and a loooooooong tail.

So why not chop it into smaller pieces and perhaps - even - document _at least_ the public API?

Who's with me?

Ps.: consistent code formatting and moar use of Task.async() would make me yay even more!

Comment 1

[Matthew N. [:MattN]]

My main complaints with MozLoopService.jsm are: 
* the globals e.g. gHawkClient which should be members on an object e.g. MozLoopServiceInternal so they're easier to access from tests
* the separation between MozLoopService and MozLoopServiceInternal is a footgun because people get |this| mixed up (e.g. bug 1071835), makes things harder to test and doesn't actually prevent access to internal state. Properties on the current *Internal can be prefixed with an underscore instead.

Note that more files == more compartments so we shouldn't split it too much.

I think it would be okay to do this once we're done development on 35 (i.e. 36+) including all uplifts otherwise it will cause uplift hell and slow us down.

Comment 2

[Matthew N. [:MattN]]

The various bits of registration code need to be cleaned up (at least renamed) as they are quite confusing:
register => promiseRegisteredWithServers => onPushRegistered => registerWithLoopServer

The middle two above are related to the push server but the 2nd doesn't mention push in the name, the last is for the loop server and the first is a talking about Loop in general I guess.

Then there is also gRegisteredDeferred which is created in promiseRegisteredWithServers (for push registration) but gets resolved when loop registration completes. I don't think onPushRegistered should call registerWithLoopServer directly so we can properly fix bug 1076428 since right now it doesn't know whether to do a Guest or FxA registration.

The way some of the mock objects are passed around multiple levels is also messy IMO. I would rather those are set as a global or a property.

Comment 3

[:shell escalante]

Moving these to blocking-loop="tech-debt" so they can be triaged against other tech debt bugs.  When we take a tech-debt bug to work on - just mark it "firefox-backlog"+ and give priority.  Then we'll pull it during iteration planning (or factor it into workload if someone is already working on).

Comment 4

[Ian Bicking (:ianbicking)]

Mike: does this bug still seem valid?

Comment 5

[Mike de Boer [:mikedeboer]]

Yes, MozLoopService is still a mess, sort of.

Comment 6

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.