Implement a non-persistent store of rooms

RESOLVED FIXED in Firefox 35

Status

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: pkerr)

Tracking

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

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 attachment, 18 obsolete attachments)

35.37 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
No description provided.
Reporter

Updated

5 years ago
User Story: (updated)
Reporter

Updated

5 years ago
Blocks: 1074665
Reporter

Updated

5 years ago
Severity: normal → enhancement
Assignee: nobody → mdeboer
Iteration: --- → 35.3
Status: NEW → ASSIGNED
Posted 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-
Reporter

Comment 3

5 years ago
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+
Posted patch WIP patch (obsolete) — Splinter Review
Attachment #8498880 - Attachment is obsolete: true
Posted patch WIP patch (obsolete) — Splinter Review
Attachment #8499568 - Attachment is obsolete: true
Assignee

Comment 6

5 years ago
Posted patch roomsStore WIP (obsolete) — Splinter Review
WIP - Integrated MozLoopPushHadler with LoopRooms to register for rooms notification
Assignee

Updated

5 years ago
Attachment #8500477 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Assignee: mdeboer → pkerr
Iteration: 35.3 → ---

Updated

5 years ago
backlog: --- → Fx35+
Assignee

Comment 7

5 years ago
Posted patch roomsStore WIP (obsolete) — Splinter Review
WIP
Assignee

Updated

5 years ago
Attachment #8503428 - Attachment is obsolete: true
Assignee

Comment 8

5 years ago
Posted 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.
Assignee

Updated

5 years ago
Attachment #8505180 - Attachment is obsolete: true
Assignee

Comment 9

5 years ago
Posted patch roomsStore WIP (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Attachment #8505668 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8505673 - Flags: feedback?(mdeboer)
Assignee

Comment 10

5 years ago
Posted patch roomsStore WIP (obsolete) — Splinter Review
fix inadvertent global
Assignee

Updated

5 years ago
Attachment #8505673 - Attachment is obsolete: true
Attachment #8505673 - Flags: feedback?(mdeboer)
Assignee

Updated

5 years ago
Attachment #8505706 - Flags: feedback?(mdeboer)
Assignee

Comment 11

5 years ago
Posted patch roomsStore WIP (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Attachment #8505706 - Attachment is obsolete: true
Attachment #8505706 - Flags: feedback?(mdeboer)
Assignee

Updated

5 years ago
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)
Assignee

Updated

5 years ago
Attachment #8505707 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
completed with fixes and feedback
Assignee

Updated

5 years ago
Attachment #8507085 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8507604 - Flags: review?(mdeboer)
Assignee

Updated

5 years ago
Depends on: 1074663
Assignee

Updated

5 years ago
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-
Assignee

Comment 16

5 years ago
fix mochi test issues
Assignee

Updated

5 years ago
Attachment #8507604 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
WIP
Assignee

Updated

5 years ago
Attachment #8508799 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
Attachment #8509707 - Attachment is obsolete: true
Assignee

Comment 19

5 years ago
Attachment #8509882 - Attachment is obsolete: true
Assignee

Updated

5 years ago
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)
Assignee

Comment 25

5 years ago
Assignee

Comment 26

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

Updated

5 years ago
Assignee: mdeboer → pkerr
Assignee

Updated

5 years ago
Attachment #8510292 - Attachment is obsolete: true
Assignee

Comment 28

5 years ago
Fixed minor bit rot. Added final review comments.
Assignee

Updated

5 years ago
Attachment #8510335 - Attachment is obsolete: true
Assignee

Comment 29

5 years ago
Carrying forward r+
Assignee

Updated

5 years ago
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: 5 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.