1161710 - Hello doesn't notify you if there is someone already in the room when you start Firefox

Status: RESOLVED INCOMPLETE

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

Description

[Mark Banner (:standard8)]

Currently, if you start Firefox when there is someone already in one of your rooms, you don't get notified about it.

The only time you get to know is if you open the panel when the other user still happens to be in your room.

I worked this out whilst looking at bug 1151862.

I need to discuss with the push folks about future plans - the push migration might help with fixing this in an "easy" fashion. Hence tentatively assigning to myself to sort out a potential solution.

Comment 1

[Mark Banner (:standard8)]

Just thinking about this a bit: Rather than rely on the push servers (which would need extra work at the moment as we're not storing the uaid across sessions), we could just do a GET /rooms just after registration on startup.

We already limit registration to the cases where a user is either non-account and has created a room, or is signed-in. So we would only be doing the get for those users that have got rooms/accounts.

The additional side effect of this, is when the user does open the Hello panel for the first time, the room list is already loaded, so its much faster populating and loading.

Adam/Remy: Do you think there's any server performance concerns if we did the additional GET on startup? Or is there a better way we can handle this?

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP Add a get all rooms call on startup

Example patch so I don't loose it.

Comment 3

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #1)
> Adam/Remy: Do you think there's any server performance concerns if we did
> the additional GET on startup? Or is there a better way we can handle this?

I'm not too worried about the GET, as long as we use the same criteria for it as we do for push endpoint registration.

If load starts getting too high, we can contemplate optimizations, such as returning room summaries in response to the registration requests.

> The additional side effect of this, is when the user does open the Hello
> panel for the first time, the room list is already loaded, so its much
> faster populating and loading.

I thought we'd spoken about this in the past -- this is something I want to happen regardless. As far as I'm concerned, getting the room list on startup kills two birds with one stone.

Comment 4

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #2)
> Created attachment 8643679 [details] [diff] [review]
> WIP Add a get all rooms call on startup
> 
> Example patch so I don't loose it.

I presume you'll be doing this for account-associated rooms also, right?

Comment 5

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #4)
> (In reply to Mark Banner (:standard8) from comment #2)
> > Created attachment 8643679 [details] [diff] [review]
> > WIP Add a get all rooms call on startup
> > 
> > Example patch so I don't loose it.
> 
> I presume you'll be doing this for account-associated rooms also, right?

Ah, never mind. I read the patch too quickly.

Comment 6

[Rémy Hubscher (:natim)]

I agree with Adam that we should update the room list on startup.

Comment 7

[Mark Banner (:standard8)]

Unassigning as I'm not working on this. See user story for what needs doing.

I think we need to make sure that the getAll doesn't throw if it fails, and probably that the getAll doesn't block the registration process.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: fetch the list of rooms from the server whenever a channel registers successfully

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8690831 [details] [diff] [review]
Patch v2: fetch the list of rooms from the server whenever a channel registers successfully

Review of attachment 8690831 [details] [diff] [review]:
-----------------------------------------------------------------

So I like this, and I can't easily think of a different fix. Definitely nice to improve panel opening time when opening after registration has taken place.

The other part of this bug that's not included in this patch, is actually doing a notification if someone is already in the room. I forgot to mention that previously, though it is in the title.

I'm quite happy for that to be split out to a separate bug, or for a second patch on this bug, your call.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8690831 [details] [diff] [review]
Patch v2: fetch the list of rooms from the server whenever a channel registers successfully

This is actually causing xpcshell failures:

 0:07.46 LOG: Thread-47 ERROR  A promise chain failed to handle a rejection: Error: 404 - rejection date: Tue Nov 24 2015 15:02:04 GMT+0000 (GMT)
MozLoopServiceInternal.createNotificationChannel/</onRegistered@resource:///modules/loop/MozLoopService.jsm:419:18
mockPushHandler.register@/Users/mark/loop/gecko-dev/objdir-ff/_tests/xpcshell/browser/components/loop/test/xpcshell/head.js:122:5
MozLoopServiceInternal.createNotificationChannel/<@resource:///modules/loop/MozLoopService.jsm:425:7
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:385:5
MozLoopServiceInternal.createNotificationChannel@resource:///modules/loop/MozLoopService.jsm:415:1
MozLoopServiceInternal.promiseRegisteredWithServers@resource:///modules/loop/MozLoopService.jsm:455:20
this.MozLoopService.promiseRegisteredWithServers@resource:///modules/loop/MozLoopService.jsm:1450:12
test_register_websocket_success_loop_server_fail@/Users/mark/loop/gecko-dev/objdir-ff/_tests/xpcshell/browser/components/loop/test/xpcshell/test_loopservice_registration.js:22:3
_run_next_test@/Users/mark/loop/gecko-dev/testing/xpcshell/head.js:1475:11
do_execute_soon/<.run@/Users/mark/loop/gecko-dev/testing/xpcshell/head.js:660:9
_do_main@/Users/mark/loop/gecko-dev/testing/xpcshell/head.js:208:5
_execute_test@/Users/mark/loop/gecko-dev/testing/xpcshell/head.js:520:5

Comment 11

[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.