Closed Bug 1089547 Opened 5 years ago Closed 5 years ago

Improve handling for errors when creating a room

Categories

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

defect
Points:
5

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
Iteration: --- → 36.2
Mike -- Does this bug replace the work that would have been done on Bug 1083340?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
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.
Attachment #8512729 - Attachment is obsolete: true
Attachment #8512729 - Flags: review?(standard8)
Attachment #8512837 - Flags: review?(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.
Attachment #8512837 - Flags: review?(standard8) → review-
backlog: --- → Fx35+
Priority: -- → P1
Attachment #8512837 - Attachment is obsolete: true
Attachment #8513409 - Flags: review?(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
Attachment #8513409 - Flags: review?(standard8) → review+
Attachment #8513564 - Flags: review?(standard8)
Attachment #8513564 - Flags: review?(standard8) → review+
Other things are already dealt with in other bugs. We're done here.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Attachment #8513409 - Flags: approval-mozilla-aurora?
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
Attachment #8513564 - Flags: approval-mozilla-aurora?
Attachment #8513409 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8513564 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please provide some qa testing steps, or mark this as [qe-verify-] if it's not testable.
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.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.