Closed Bug 1074664 Opened 6 years ago Closed 6 years ago

Implement a non-persistent store of rooms

Categories

(Hello (Loop) :: Client, enhancement)

enhancement
Not set
normal
Points:
3

Tracking

(firefox35 fixed, firefox36 fixed)

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

People

(Reporter: standard8, Assigned: pkerr)

References

Details

(Whiteboard: [rooms])

User Story

* After registration, for guest and optionally FxA, obtain the list of rooms for both

https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms

* Store the list in a chrome/backend based object
* Make the list accessible via the MozLoopAPI - for all room details, or just a specified room

Attachments

(1 file, 18 obsolete files)

35.37 KB, patch
jesup
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
Blocks: 1074665
Severity: normal → enhancement
Assignee: nobody → mdeboer
Iteration: --- → 35.3
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
Mark, is this at all looking like something in the direction that I'm supposed to be heading?
Attachment #8498880 - Flags: feedback?(standard8)
Flags: needinfo?(mmucci)
Added to IT 35.3.  Mike, can you provide a point value.
Flags: needinfo?(mmucci)
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 3
Flags: qe-verify? → qe-verify-
Comment on attachment 8498880 [details] [diff] [review]
WIP patch

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

Yes, this looks good.
Attachment #8498880 - Flags: feedback?(standard8) → feedback+
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #8498880 - Attachment is obsolete: true
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #8499568 - Attachment is obsolete: true
Attached patch roomsStore WIP (obsolete) — Splinter Review
WIP - Integrated MozLoopPushHadler with LoopRooms to register for rooms notification
Attachment #8500477 - Attachment is obsolete: true
Assignee: mdeboer → pkerr
Iteration: 35.3 → ---
backlog: --- → Fx35+
Attached patch roomsStore WIP (obsolete) — Splinter Review
WIP
Attachment #8503428 - Attachment is obsolete: true
Attached patch roomsStore WIP (obsolete) — Splinter Review
Fixed MozLoop API injection double-cloning. Moved non-conforming functions (eg. the notification callback) out of LoopRoomsAPI and into a separate exported service API.
Attachment #8505180 - Attachment is obsolete: true
Attached patch roomsStore WIP (obsolete) — Splinter Review
Attachment #8505668 - Attachment is obsolete: true
Attachment #8505673 - Flags: feedback?(mdeboer)
Attached patch roomsStore WIP (obsolete) — Splinter Review
fix inadvertent global
Attachment #8505673 - Attachment is obsolete: true
Attachment #8505673 - Flags: feedback?(mdeboer)
Attachment #8505706 - Flags: feedback?(mdeboer)
Attached patch roomsStore WIP (obsolete) — Splinter Review
Attachment #8505706 - Attachment is obsolete: true
Attachment #8505706 - Flags: feedback?(mdeboer)
Attachment #8505707 - Flags: feedback?(mdeboer)
Comment on attachment 8505707 [details] [diff] [review]
roomsStore WIP

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

Cancelling feedback until a proper patch is attached.

::: browser/components/loop/LoopRooms.jsm
@@ +6,5 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/loop/MozLoopService.jsm");

Please make these three lazy module getters.

@@ +25,5 @@
> +
> +let gRoomsListFetched = false;
> +let gRooms = new Map();
> +
> +let LoopRoomsInternal = Object.freeze({

You can freeze this object after you declared it.

@@ +29,5 @@
> +let LoopRoomsInternal = Object.freeze({
> +  getAll: function(callback) {
> +    MozLoopService.register().then(Task.async(function*() {
> +      if (gRoomsListFetched) {
> +        return;

Shouldn't you return the list of rooms via the callback?

@@ +34,5 @@
> +      }
> +      // Fetch the rooms from the server.
> +      let sessionType = MozLoopService.userProfile ? LOOP_SESSION_TYPE.FXA :
> +                        LOOP_SESSION_TYPE.GUEST;
> +      let rooms = yield this._requestRoomList(sessionType);

underscore not necessary for methods attached to LoopRoomsInternal.
Attachment #8505707 - Flags: feedback?(mdeboer)
Attachment #8505707 - Attachment is obsolete: true
completed with fixes and feedback
Attachment #8507085 - Attachment is obsolete: true
Attachment #8507604 - Flags: review?(mdeboer)
Depends on: 1074663
Blocks: 1074699
Comment on attachment 8507604 [details] [diff] [review]
Implement a non-persistent store of rooms

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

Again, this looks like an interdiff. Can you please post the full patch?

As a general note: I don't understand why we're requesting room details for each room that we receive... This will make the HTTP request count grow linearly each time the list grows.
1) This added latency will kill perf. With a list of 50 rooms, we'll need to wait for 50 + 1 requests before we have any data in.
2) The initial payload of the getAll() result will contain a lot of details that we don't need to build the initial list. If there _IS_ data missing, we need to report that to the server team to get fixed.

I propose to decouple fetching the list and the room details to make sure we lazy-load as much as possible. We can always cache the results once they are received on the 'details' field of a room.

::: browser/components/loop/LoopRooms.jsm
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +// See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
> +const PREF_LOG_LEVEL = "loop.debug.loglevel";

nit: you're using this string only once, so you might as well keep it inside the Log boilerplate code.

@@ +9,3 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +Cu.import("resource://gre/modules/Task.jsm");

Please add more imports below the generic ones, like 'Services.jsm', 'XPCOMUtils', etc.

Also, please use the lazy module getter pattern as much as possible.

@@ +12,4 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Services is commonly used and already compartmentized when this module is evaluated; no need to make its getter lazy.

@@ +53,5 @@
> +        // Next, request the detailed information for each room.
> +        // If the request fails the room data will not be added to the map.
> +        try {
> +          let details = yield this.requestRoomDetails(room.roomToken, sessionType);
> +          for (let attr in details) {room[attr] = details[attr]}

Please correct the formatting here.

@@ +60,5 @@
> +        catch (error) {log.warn("failed GETing room details for roomToken = " + room.roomToken + ": ", error)}
> +      }
> +      return [...gRooms.values()];
> +      }.bind(this)).then(
> +        (roomList) => {callback(null, roomList)})

please just return the result directly in the iterator function above.

@@ +62,5 @@
> +      return [...gRooms.values()];
> +      }.bind(this)).then(
> +        (roomList) => {callback(null, roomList)})
> +    ).catch((error) => {log.error("getAll error:", error);
> +            callback(error)});

You can write this as:

```js
}.bind(this)).catch(error => {
  log.error("getAll error:", error);
  callback(error);
});
```

@@ +70,5 @@
> +  getRoomData: function(roomID, callback) {
> +    if (gRooms.has(roomID)) {
> +      callback(null, gRooms.get(roomID));
> +    } else {
> +      callback(new Error("invalid localRoomID"));

This error doesn't really tell what actually happening, I think; you might want to change it to something like `"Room data not found or not fetched yet for room with ID " + roomID`
I think that it's OK, generally, to be a bit verbose with error messages. Just like with commit messages :)

@@ +78,5 @@
> +
> +  /**
> +   * Request list of all rooms associated with this account.
> +   *
> +   * @param {Object} sessionType Indicates which hawkRequest endpoint to use.

I think this is a 'String'.

@@ +82,5 @@
> +   * @param {Object} sessionType Indicates which hawkRequest endpoint to use.
> +   *
> +   * @returns {Promise} room list
> +   */
> +  requestRoomList: function(sessionType) {

You can make this a `Task.async(function* (sessionType) {})` to make this read a bit cleaner.

@@ +90,5 @@
> +        if (!Array.isArray(roomsList)) {
> +          throw new Error("Missing array of rooms in response.");
> +        }
> +        return roomsList;
> +      });

You're missing an error handler here. If you want this method to keep throwing errors, please put the callee in a try...catch block.

@@ +97,5 @@
> +  /**
> +   * Request information about a specific room from the server.
> +   *
> +   * @param {Object} token Room identifier returned from the LoopServer.
> +   *

nit: unnecessary newline

@@ +104,5 @@
> +   * @returns {Promise} room details
> +   */
> +  requestRoomDetails: function(token, sessionType) {
> +    return MozLoopService.hawkRequest(sessionType, "/rooms/" + token, "GET")
> +      .then(response => {return JSON.parse(response.body);});

Curly braces are not necessary here.

@@ +108,5 @@
> +      .then(response => {return JSON.parse(response.body);});
> +  },
> +
> +};
> +Object.freeze("LoopRoomsInternal");

You'll want this to be `Object.freeze(LoopRoomsInternal);` as you're now freezing a String.

@@ +120,2 @@
>   */
> +this.LoopRoomsAPI = {

Please merge this with `this.LoopRooms`

@@ +147,5 @@
> +};
> +Object.freeze("LoopRoomsAPI");
> +
> +/* Exported services (not part of the API reflected through MozLoop.rooms to content
> +   via MozLoopAPI.). Used by other JSMs.

I really don't like this distinction. Let's just keep this singular.

@@ +154,1 @@
>  // Channel ids that will be registered with the PushServer for notifications

nit: indentation

@@ +160,5 @@
> +  /**
> +   * Callback used to indicate changes to rooms data on the LoopServer.
> +   *
> +   * @param {Object} version Version number assigned to this change set.
> +   *

nit: unnecessary newline.

@@ +168,5 @@
>    onNotification: function(version, channelID) {
>      return;
>    },
> +};
> +Object.freeze("LoopRooms");

Again, freezing a String like this doesn't do what you'd expect.
Attachment #8507604 - Flags: review?(mdeboer) → feedback+
Attachment #8507604 - Flags: feedback+ → feedback-
fix mochi test issues
Attachment #8507604 - Attachment is obsolete: true
WIP
Attachment #8508799 - Attachment is obsolete: true
Attachment #8509707 - Attachment is obsolete: true
Attachment #8509882 - Attachment is obsolete: true
Attachment #8509940 - Flags: review?(mdeboer)
Comment on attachment 8509940 [details] [diff] [review]
Implement a non-persistent rooms store

Paul, I don't quite understand why you keep insisting on putting partial patches up for review based on work that hasn't landed yet... I thought we discussed this earlier?

It's quite impossible to properly review this patch, even though the LoopRooms part is starting to look very good.
Attachment #8509940 - Flags: review?(mdeboer) → review-
Try push for this patch, mostly to ensure that this doesn't break hawk-client consumers since I made a small change in it.
Assignee: pkerr → mdeboer
Attachment #8509940 - Attachment is obsolete: true
Attachment #8510291 - Flags: review?(MattN+bmo)
Iteration: --- → 36.1
Flags: needinfo?(mmucci)
Fixed a few whoopsies.
Attachment #8510291 - Attachment is obsolete: true
Attachment #8510291 - Flags: review?(MattN+bmo)
Attachment #8510292 - Flags: review?(MattN+bmo)
Added to IT 36.1
Flags: needinfo?(mmucci)
Uploaded a copy from git branch to work on merge and bit-rot
Attachment #8510292 - Flags: review?(standard8)
Attachment #8510292 - Flags: review?(standard8)
Attachment #8510292 - Flags: review?(MattN+bmo)
Comment on attachment 8510335 [details] [diff] [review]
Implement a non-persistent rooms store

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

r=me with comments addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +4,5 @@
>  "use strict";
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +Cu.import("resource://gre/modules/Task.jsm");

Please make this a lazy getter.

@@ +11,2 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "MozLoopService",

Perhaps it's good to say:
```js
const {MozLoopService, LOOP_SESSION_TYPE} = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
```

@@ +66,5 @@
> +            room[attr] = details[attr]
> +          }
> +          gRooms.set(id, room);
> +        }
> +        catch (error) {log.warn("failed GETing room details for roomToken = " + room.roomToken + ": ", error)}

please change this to 
```js
} catch (error) {
  log.warn("...");
}
```

@@ +72,5 @@
> +      callback(null, [...gRooms.values()]);
> +      return;
> +      }.bind(this))
> +    ).catch((error) => {log.error("getAll error:", error);
> +            callback(error)});

Please change this to
```js
).catch(error => {
  log.error("...");
  callback(error);
});
```

@@ +102,5 @@
> +          // To be caught by the caller using the returned Promise.
> +          throw new Error("Missing array of rooms in response.");
> +        }
> +        return roomsList;
> +      });

Please add
```js
then(response => {
  //...
}, err => callback(err)).catch(ex => callback(ex));
```

@@ +115,5 @@
> +   * @returns {Promise} room details
> +   */
> +  requestRoomDetails: function(token, sessionType) {
> +    return MozLoopService.hawkRequest(sessionType, "/rooms/" + token, "GET")
> +      .then(response => JSON.parse(response.body));

tricky to do error handler here, since you added a thenable here.

::: browser/components/loop/MozLoopAPI.jsm
@@ +10,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/loop/LoopCalls.jsm");
>  Cu.import("resource:///modules/loop/MozLoopService.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");

Please make this lazy.

@@ +11,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/loop/LoopCalls.jsm");
>  Cu.import("resource:///modules/loop/MozLoopService.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");
> +Cu.import("resource:///modules/loop/LoopContacts.jsm");

duplicate entry.

::: browser/components/loop/test/xpcshell/head.js
@@ +9,5 @@
>  Cu.import("resource://testing-common/httpd.js");
>  Cu.import("resource:///modules/loop/MozLoopService.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource:///modules/loop/LoopCalls.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");

This doesn't need to be included in head.js

::: browser/components/loop/test/xpcshell/test_rooms_getdata.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://services-common/utils.js");

Please look at the unit test I attached in the patch v2.1, you might want to yank some things out there. Functionally, this looks good to me.
Attachment #8510335 - Flags: review+
Assignee: mdeboer → pkerr
Attachment #8510292 - Attachment is obsolete: true
Fixed minor bit rot. Added final review comments.
Attachment #8510335 - Attachment is obsolete: true
Carrying forward r+
Keywords: checkin-needed
Attachment #8510535 - Attachment description: Implement a non-persistent rooms store → [landed on fx-team] Implement a non-persistent rooms store
https://hg.mozilla.org/mozilla-central/rev/b20cca81a9cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1087041
No longer blocks: 1074665
(In reply to Carsten Book [:Tomcat] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/b20cca81a9cf

This is mozilla36.
Target Milestone: mozilla35 → mozilla36
Comment on attachment 8510535 [details] [diff] [review]
[landed on fx-team] Implement a non-persistent rooms store

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8510535 - Flags: review+
Attachment #8510535 - Flags: approval-mozilla-aurora?
Attachment #8510535 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.