1089547 - Improve handling for errors when creating a room

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

When creating a room, the mozLoopAPI will automatically open a chat window. If the room create fails, this will mean we either have a "flash" of a chat window, or we'll end up displaying errors in two places.

Additionally, there's some extra complexity caused by this that would make handling the room information simpler across desktop.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

My 2 cents: we should reconsider opening that window immediately; for the time being, we should just wait for the created room information being retrieved from the server. This will ease implementation a lot, and we'll still be able to improve UX regarding latency later.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: simplify LoopRooms implementation, add support for events

Comment 3

[Maire Reavy [:mreavy]]

Mike -- Does this bug replace the work that would have been done on Bug 1083340?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #3)
> Mike -- Does this bug replace the work that would have been done on Bug
> 1083340?

No.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8512729 [details] [diff] [review]
Part 1: simplify LoopRooms implementation, add support for events

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

::: browser/components/loop/LoopRooms.jsm
@@ +20,5 @@
> +XPCOMUtils.defineLazyGetter(this, "log", () => {
> +  let ConsoleAPI = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).ConsoleAPI;
> +  let consoleOptions = {
> +    maxLogLevel: Services.prefs.getCharPref("loop.debug.loglevel").toLowerCase(),
> +    prefix: "Loop",

You can just access the log property of MozLoopService IIRC so you don't need to duplicate this.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Part 1.1: simplify LoopRooms implementation, add support for events

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8512837 [details] [diff] [review]
Part 1.1: simplify LoopRooms implementation, add support for events

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

This would be r+ with comments, but its failing mochitests tests, so I can't give it my normal final run. Here's the failure output from the tests:

The following tests failed:
913 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | uncaught exception - Error: Permission denied to pass a Function via structured clone at resource:///modules/loop/MozLoopAPI.jsm:179
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1346
null:null:0
914 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | uncaught exception - Script error. at chrome://browser/content/loop/libs/l10n.js:0
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1346
null:null:0
915 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:28 - TypeError: loopDoc.querySelector(...) is null
Stack trace:
    checkFxA401@chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:28:6
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
    Tester_execTest@chrome://mochikit/content/browser-test.js:646:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:572:7
    SimpleTest.waitForFocus/maybeRunTests/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:683:49
916 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:28 - TypeError: loopDoc.querySelector(...) is null
Stack trace:
    checkFxA401@chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:28:6
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
    Tester_execTest@chrome://mochikit/content/browser-test.js:646:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:572:7
    SimpleTest.waitForFocus/maybeRunTests/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:683:49
917 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | Test timed out - expected PASS
918 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:269 - TypeError: visibleEmail is undefined
Stack trace:
    basicAuthorizationAndRegistration@chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js:269:3
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:868:21
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
    Tester_execTest@chrome://mochikit/content/browser-test.js:646:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:572:7
    SimpleTest.waitForFocus/maybeRunTests/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:683:49
919 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_loop_fxa_server.js | A promise chain failed to handle a rejection:  - at chrome://mochikit/content/browser-test.js:653 - TypeError: this.SimpleTest.isExpectingUncaughtException is not a function
Stack trace:
Tester_execTest/<@chrome://mochikit/content/browser-test.js:653:34
Tester_execTest@chrome://mochikit/content/browser-test.js:646:9
Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:572:7
SimpleTest.waitForFocus/maybeRunTests/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:683:49

::: browser/components/loop/LoopRooms.jsm
@@ +21,5 @@
> +const roomsPushNotification = function(version, channelID) {
> +  return LoopRoomsInternal.onNotification(version, channelID);
> +};
> +
> +// Since this the gRooms map as defined below is a local cache of room object that

Couple of language nits: Since the gRooms map as defined below is a local cache of room objects that

@@ +30,1 @@
>  let gRooms = new Map();

These feel like they would be better encapsulated as part of LoopRoomsInternal

@@ +92,5 @@
>          try {
> +          yield LoopRooms.promise("get", room.roomToken);
> +        } catch (error) {
> +          MozLoopService.log.error("Failed to retrieve room details for room '" +
> +                                   room.roomToken + "'", error);

Should we early return or throw the error here? Otherwise we're going to emit the event and clear the dirty flag.

@@ +198,3 @@
>   */
>  this.LoopRooms = {
> +  getAll: function(version, callback) {

I'm thinking the jsdocs should be on these functions as they are the public api - if we're only going to have them in one place.

If not, I think there should be a comment at the start here to say "see the internal code for the api docs".

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +5,5 @@
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource:///modules/Chat.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");
> +
> +const openChatOrig = Chat.open;

No need for the Chat.open and the cleanup for it - probably don't even need to include Chat.jsm.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Part 1.2: simplify LoopRooms implementation, add support for events

Comment 9

[Mike de Boer [:mikedeboer]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46ca2c4f78b2

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8513409 [details] [diff] [review]
Part 1.2: simplify LoopRooms implementation, add support for events

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

Looks good, r=Standard8

Comment 11

[Mike de Boer [:mikedeboer]]

Thanks! Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/bf096384153d

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Part 2: follow-up

Comment 13

[Mike de Boer [:mikedeboer]]

Pushed follow-up to fx-team as https://hg.mozilla.org/integration/fx-team/rev/2ffc91152d91

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf096384153d
https://hg.mozilla.org/mozilla-central/rev/2ffc91152d91

Comment 15

[Mike de Boer [:mikedeboer]]

Other things are already dealt with in other bugs. We're done here.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b74d6c49038d
https://hg.mozilla.org/releases/mozilla-aurora/rev/6dca7a6a8f33

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8513409 [details] [diff] [review]
Part 1.2: simplify LoopRooms implementation, add support for events

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8513564 [details] [diff] [review]
Part 2: follow-up

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 19

[Paul Silaghi, QA [:pauly]]

Please provide some qa testing steps, or mark this as [qe-verify-] if it's not testable.

Comment 20

[u279076]

I'm deprioritizing this for QE testing given this has some in-testsuite coverage and we don't have good instructions on what needs testing. If you think this bug is high risk to introducing regressions in Firefox 35 then please reflag this bug for QE verification and provide more information.

Thank you.