As a user, when I create a room, I should see the new room open in a conversation window (CreateRoom)

VERIFIED FIXED in Firefox 35

Status

--
enhancement
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: pkerr)

Tracking

unspecified
mozilla36
Points:
5
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox35 verified, firefox36 verified)

Details

(Whiteboard: [rooms])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
As a user, when I create a room, I should see the new room open in a conversation window, so that I can check my camera and settings straight away

UX:
* When a new room is created, open it in the browser, if the browser created the room (e.g. rather than a different log-in)
No longer depends on: 1074682

Updated

4 years ago
backlog: --- → Fx35+
This really means implementing mozLoop.rooms.CreateRoom and at least the RoomCreationError callback.  This is reasonably well described in the chicken-scratchings at https://etherpad.mozilla.org/loop-rooms-rest-stuff between lines 28 and 38.

Once this lands, the patches in bug 1074680 and bug 1074686 (if they're already in the tree by then) should Just Start Working, as they've been writte-to and tested against a fake version of that API.
Summary: As a user, when I create a room, I should see the new room open in a conversation window → As a user, when I create a room, I should see the new room open in a conversation window (CreateRoom)
No longer depends on: 1074686
(Assignee)

Updated

4 years ago
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8505885 [details] [diff] [review]
WIP

Initial createRoom and AddCallback implimentation.
(Assignee)

Comment 3

4 years ago
Created attachment 8507086 [details] [diff] [review]
Add createRoom and addCallback to LooRooms API.
(Assignee)

Updated

4 years ago
Attachment #8505885 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8507605 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API
(Assignee)

Updated

4 years ago
Attachment #8507086 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8507605 - Flags: review?(dmose)
(Assignee)

Comment 5

4 years ago
Created attachment 8507982 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

changed reference to roomLocalID to localRoomID
(Assignee)

Updated

4 years ago
Attachment #8507605 - Attachment is obsolete: true
Attachment #8507605 - Flags: review?(dmose)
(Assignee)

Updated

4 years ago
Attachment #8507982 - Flags: review?(dmose)
(Assignee)

Comment 6

4 years ago
Created attachment 8507984 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

changed reference to roomLocalID to localRoomID in tests
(Assignee)

Updated

4 years ago
Attachment #8507982 - Attachment is obsolete: true
Attachment #8507982 - Flags: review?(dmose)
(Assignee)

Updated

4 years ago
Attachment #8507984 - Flags: review?(dmose)
(Assignee)

Updated

4 years ago
Depends on: 1074664
Paul and I have started reviewing this, and there's a rebased version of the patch (along with the most up-to-date (as of this writing) versions of the patches for bugs 663 and 664).  Paul is going to work with Matt on sorting out remaining 664 review issues, and I'm going to work on another review in my queue, so the tentative plan (subject to other decisions on how to move the rooms work forward) is that Paul and I may get back to this tomorrow (Thursday).
(Assignee)

Comment 8

4 years ago
Created attachment 8510571 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

rebased to apply after fx-team (assumed to already incorporate 1074663) plus 1074664: fx-team -> 664 -> 699
(Assignee)

Updated

4 years ago
Attachment #8507984 - Attachment is obsolete: true
Attachment #8507984 - Flags: review?(dmose)
(Assignee)

Updated

4 years ago
Attachment #8510571 - Flags: review?(dmose)
Comment on attachment 8510571 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

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

Looks good; r=dmose with the various concerns appropriately addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +125,5 @@
> +
> +  createRoom: function(props, callback) {
> +    // Always create a basic room record and launch the window, attaching
> +    // the roomID. Later errors will be returned via the registered callback.
> +    let roomID = MozLoopService.generateLocalID((id) => {gRooms.has(id)})

Renaming this to localRoomId will make it consistent down the stack.

@@ +130,5 @@
> +    let room = {localRoomID : roomID};
> +    for (let prop in props) {
> +      room[prop] = props[prop]
> +    }
> +    delete room.expiresIn; //Do not keep this value - it is a request to the server

Good plan, but let's delay this until after we've sent it  :-)

@@ +140,5 @@
> +        !"expiresIn" in props ||
> +        !"roomOwner" in props ||
> +        !"maxSize" in props) {
> +      this.postCallback(roomID, "RoomCreated",
> +                         new Error("missing required room create property"));

Nit: indented 2 spaces too far.

@@ +141,5 @@
> +        !"roomOwner" in props ||
> +        !"maxSize" in props) {
> +      this.postCallback(roomID, "RoomCreated",
> +                         new Error("missing required room create property"));
> +      return null;

This wants to return the room ID.

@@ +168,5 @@
> +   * for a callback type. Callback must be of the form:
> +   *    function (error, success) {...}
> +   *
> +   * @param {Object} roomID Local room identifier.
> +   * @param {Object} callbackName callback type

Strings

@@ +170,5 @@
> +   *
> +   * @param {Object} roomID Local room identifier.
> +   * @param {Object} callbackName callback type
> +   * @param {Object} error result or null.
> +   * @param {Object} success result or null.

@param {?Object} on one or both of those.  Also perhaps worth noting that they can't BOTH be null.

@@ +179,5 @@
> +      // No callbacks have been registered or results posted for this room.
> +      // Initialize a record for this room and callbackName.
> +      gCallbacks.set(roomID, new Map([[
> +        callbackName,
> +        {callbacks: [], result: {error: error, success: success}}]])); 

Trailing whitespace.

@@ +184,5 @@
> +      return;
> +    }
> +
> +    let namedCallback = roomCallbacks.get(callbackName);
> +    // A callback of this name has not been registered.

It'd be nice if the commentary was more explicit about the queueing of the result, mostly in the jsdoc, but a little here too.  Maybe just variable names here, I'm not sure.  savedResult?

@@ +208,5 @@
> +      // No callbacks have been registered or results posted for this room.
> +      // Initialize a record for this room and callbackName.
> +      gCallbacks.set(roomID, new Map([[
> +        callbackName,
> +        {callbacks: [callback]}]])); 

A little  more commentary and/or renaming here.  Eg example concrete record, and/or callback -> callbackFn.

@@ +213,5 @@
> +      return;
> +    }
> +
> +    let namedCallback = roomCallbacks.get(callbackName);
> +    // A callback of this name has not been registered.

Consider whether a few of the lower-level operations here and elsewhere might want to be hoisted into their own helper functions, so that functions like this can be written a single level of abstraction for maintainability.

@@ +222,5 @@
> +      return;
> +    }
> +      
> +    // Add this callback if not already in the array
> +    if (namedCallback.callbacks.indexOf(callback) >= 0) {

Nit: If there's a more semantic way to write this instead of indexOf, you could probably ditch the comment.

@@ +227,5 @@
> +      return;
> +    }
> +    namedCallback.callbacks.push(callback);
> +
> +    // If a result has been posted for this callback,

This is more understandable with s/for this callback,/,/

@@ +307,5 @@
> +
> +  /**
> +   * Register a callback of a specified type with a roomID.
> +   *
> +   * @param {Object} roomID Local room identifier.

localRoomId for consistency here, and throughout the rest of this block.

@@ +308,5 @@
> +  /**
> +   * Register a callback of a specified type with a roomID.
> +   *
> +   * @param {Object} roomID Local room identifier.
> +   * @param {Object} callbackName callback type

Both {Strings}

@@ +318,5 @@
> +
> +  /**
> +   * Un-register and delete a callback of a specified type for a roomID.
> +   *
> +   * @param {Object} roomID Local room identifier.

{String}, I think

@@ +319,5 @@
> +  /**
> +   * Un-register and delete a callback of a specified type for a roomID.
> +   *
> +   * @param {Object} roomID Local room identifier.
> +   * @param {Object} callbackName callback type

Also a {String}

::: browser/components/loop/test/xpcshell/test_rooms_create.js
@@ +55,5 @@
> +    let returnedID = LoopRooms.createRoom(roomProps, (error, data) => {
> +      do_check_false(error);
> +      do_check_true(data);
> +      do_check_true(hasTheseProps(roomData, data));
> +      do_check_eq(data.localRoomID, urlPieces[1]);

If you can do a global s/localRoomID/localRoomId/ that'd be great

@@ +98,5 @@
> +    // Revert original Chat.open implementation
> +    Chat.open = openChatOrig;
> +
> +    // clear test pref
> +    Services.prefs.clearUserPref("loop.seenToS");

Can we get rid of this?
Attachment #8510571 - Flags: review?(dmose) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8510762 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

Incorporate feedback comments. Carry forward r+
(Assignee)

Updated

4 years ago
Attachment #8510571 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
This should be checked into the fx-team branch.
(Assignee)

Updated

4 years ago
Points: --- → 5
Keywords: checkin-needed
(Reporter)

Comment 12

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/49647fbad84e
Iteration: --- → 36.1
Keywords: checkin-needed
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/49647fbad84e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Flags: qe-verify+
Flags: in-testsuite?
Flags: in-moztrap?
status-firefox35: --- → fixed
status-firefox36: --- → fixed
Comment on attachment 8510762 [details] [diff] [review]
Add createRoom and addCallback to LoopRooms API

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8510762 - Flags: review+
Attachment #8510762 - Flags: approval-mozilla-aurora?
Attachment #8510762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FF 35b4, 36.0a2 (2014-12-18) Ubuntu 13.04
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
status-firefox36: fixed → verified
(Reporter)

Comment 17

4 years ago
Covered by functional tests.
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-moztrap?
You need to log in before you can comment on or make changes to this bug.