Improve handling for errors when creating a room

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: standard8, Assigned: mikedeboer)

Tracking

unspecified
mozilla36
Points:
5
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Points: --- → 5
(Assignee)

Updated

4 years ago
Flags: qe-verify+
Flags: firefox-backlog+

Updated

4 years ago
Iteration: --- → 36.2
(Assignee)

Comment 2

4 years ago
Created attachment 8512729 [details] [diff] [review]
Part 1: simplify LoopRooms implementation, add support for events
Attachment #8512729 - Flags: review?(standard8)
Mike -- Does this bug replace the work that would have been done on Bug 1083340?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8512837 [details] [diff] [review]
Part 1.1: simplify LoopRooms implementation, add support for events
Attachment #8512729 - Attachment is obsolete: true
Attachment #8512729 - Flags: review?(standard8)
Attachment #8512837 - Flags: review?(standard8)
(Reporter)

Comment 7

4 years ago
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-

Updated

4 years ago
backlog: --- → Fx35+
Priority: -- → P1
(Assignee)

Comment 8

4 years ago
Created attachment 8513409 [details] [diff] [review]
Part 1.2: simplify LoopRooms implementation, add support for events
Attachment #8512837 - Attachment is obsolete: true
Attachment #8513409 - Flags: review?(standard8)
(Reporter)

Comment 10

4 years ago
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+
(Assignee)

Comment 11

4 years ago
Thanks! Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/bf096384153d
Keywords: leave-open
(Assignee)

Comment 12

4 years ago
Created attachment 8513564 [details] [diff] [review]
Part 2: follow-up
Attachment #8513564 - Flags: review?(standard8)
(Reporter)

Updated

4 years ago
Attachment #8513564 - Flags: review?(standard8) → review+
(Assignee)

Comment 15

4 years ago
Other things are already dealt with in other bugs. We're done here.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Keywords: leave-open
status-firefox35: --- → fixed
status-firefox36: --- → 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.