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

Status: VERIFIED FIXED

(Hello (Loop) :: Client, enhancement)

Description

[Mark Banner (:standard8)]

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)

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 2

[Paul Kerr [:pkerr]]

Attachment: WIP

Initial createRoom and AddCallback implimentation.

Comment 3

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LooRooms API.

Comment 4

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LoopRooms API

Comment 5

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LoopRooms API

changed reference to roomLocalID to localRoomID

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LoopRooms API

changed reference to roomLocalID to localRoomID in tests

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 8

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LoopRooms API

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

Comment 9

[Dan Mosedale (:dmosedale, :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?

Comment 10

[Paul Kerr [:pkerr]]

Attachment: Add createRoom and addCallback to LoopRooms API

Incorporate feedback comments. Carry forward r+

Comment 11

[Paul Kerr [:pkerr]]

This should be checked into the fx-team branch.

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/49647fbad84e

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49647fbad84e

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f042a59a39e

Comment 15

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

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

Comment 16

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b4, 36.0a2 (2014-12-18) Ubuntu 13.04

Comment 17

[Mark Banner (:standard8)]

Covered by functional tests.