Closed Bug 1205591 Opened 9 years ago Closed 9 years ago

Add a WebChannel to handle opening rooms

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
3

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.3 - Sep 21
Tracking Status
firefox43 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file, 2 obsolete files)

Part of bug 1154251, split out to try and ensure it lands in 43, and the standalone UI can come later.

The idea is to implement a channel which has two items:

- checkWillOpenRoom - used to check if the room will open if the user wishes it.
- openRoom - actually opens the room.

The check step is necessary as to avoid unnecessary delays when the user clicks join (because we're not guaranteed a response), hence we need to do this check whilst the page is loading getting the context data from the server.

The open room will do the main step of actually opening the room.
Although we haven't got the main part of the link clicker changes, I'm confident this is correct due to the tests. I think the archictecture should let us be flexible enough in case we haven't thought of something.
Attachment #8662271 - Flags: review?(mdeboer)
Comment on attachment 8662271 [details] [diff] [review]
Add a basic service channel implementation for listening to Loop's link clicker and opening rooms when requested.

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

Looking good already, but I do have a two or so questions that are holding it back for now.

::: browser/app/profile/firefox.js
@@ +1694,5 @@
>  
>  pref("loop.enabled", true);
>  pref("loop.textChat.enabled", true);
>  pref("loop.server", "https://loop.services.mozilla.com/v0");
> +pref("loop.link-clicker-url", "https://hello.firefox.com/");

Can you rename this to `loop.linkClicker.url`, please?

::: browser/components/loop/modules/LoopRooms.jsm
@@ +184,5 @@
>  
>    /**
> +   * Initialises the rooms, sets up the link clicker listener.
> +   *
> +   * @return {[type]} [description]

Please remove this boilerplate.

@@ +211,5 @@
> +    if (!linkClickerUrl) {
> +      return;
> +    }
> +
> +    let uri = Services.io.newURI(linkClickerUrl, null, null);

This can throw.

@@ +214,5 @@
> +
> +    let uri = Services.io.newURI(linkClickerUrl, null, null);
> +
> +    gLinkClickerChannel = new WebChannel("loop-link-clicker", uri);
> +    gLinkClickerChannel.listen(this._handleChannelMessage.bind(this));

Since we're introducing the term 'link-clicker' (which I like better than the overly generic 'standalone', btw) here, can you name the handler `_handleLinkClickerMessage`?

@@ +950,5 @@
> +
> +  /**
> +   * Handles a message received from the content channel.
> +   *
> +   * @param {Object} id              The channel id.

An Object, really?

@@ +959,5 @@
> +    if (!message) {
> +      return;
> +    }
> +
> +    function sendResponse(response) {

nit: `let sendResponse = response => { ... };`

@@ +965,5 @@
> +        response: response
> +      }, sendingContext);
> +    }
> +
> +    var hasRoom = this.rooms.has(message.roomToken);

nit: please use 'let' here.

@@ +978,5 @@
> +        }
> +        sendResponse(hasRoom);
> +        break;
> +      default:
> +        sendResponse(false);

nit: please add a `break;` here for posterity.

@@ +1100,5 @@
> +
> +  /**
> +   * Expose the internal rooms map for testing purposes only.
> +   */
> +  _internalRooms: LoopRoomsInternal.rooms

I think you'll understand when I say that I'm not fond of exposing the guts of this module publicly... Can you think of way to write the tests without needing to expose the rooms map publicly outside of tests?

::: browser/components/loop/test/mochitest/browser_LoopRooms_channel.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/*
> + * This file contains tests for various context-in-conversations helpers in
> + * LoopUI and MozLoopAPI.

Is this comment correct?

@@ +9,5 @@
> +
> +var {WebChannel} = Cu.import("resource://gre/modules/WebChannel.jsm", {});
> +var {Chat} = Cu.import("resource:///modules/Chat.jsm", {});
> +Services.scriptloader
> +  .loadSubScript("chrome://mochitests/content/browser/browser/base/content/test/general/head.js", this);

Can you explain in a comment why you need this, instead of using the BrowserTestUtils module?

@@ +73,5 @@
> +
> +add_task(function* test_loopRooms_webchannel_checkWillOpenRoom() {
> +  // We've already tested if the room doesn't exist above, so here we add the
> +  // room and check the result.
> +  LoopRooms._internalRooms.set(ROOM_TOKEN, {roomToken: ROOM_TOKEN});

nit: object literal formatting.

Why do you need to do this directly on the internal rooms object? Is the public API not enough?

@@ +98,5 @@
> +  Assert.ok(!openedUrl, "should not open a chat window");
> +  Assert.equal(got.message.response, false, "should have got a response of false");
> +
> +  // Now add a room & check it.
> +  LoopRooms._internalRooms.set(ROOM_TOKEN, {roomToken: ROOM_TOKEN});

nit: object literal formatting.

::: browser/components/loop/test/mochitest/test_loopLinkClicker_channel.html
@@ +27,5 @@
> +  var event = new window.CustomEvent("WebChannelMessageToChrome", {
> +    detail: {
> +      id: "loop-link-clicker",
> +      message: {
> +        command: "checkWillOpenRoom",

These two files are identical, except for this command part. Isn't it possible to pass the command to send back as a query param?
Attachment #8662271 - Flags: review?(mdeboer) → feedback+
Rank: 19
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> @@ +73,5 @@
> > +
> > +add_task(function* test_loopRooms_webchannel_checkWillOpenRoom() {
> > +  // We've already tested if the room doesn't exist above, so here we add the
> > +  // room and check the result.
> > +  LoopRooms._internalRooms.set(ROOM_TOKEN, {roomToken: ROOM_TOKEN});
>
> Why do you need to do this directly on the internal rooms object? Is the
> public API not enough?

The public API could be enough, but then we get into how much of the code we actually want to simulate & test - we can't mock MozLoopService.hawkRequest due to the frozen interfaces, so then about the only way to do it would be to replicate/share the code of the http server from the xpcshell-tests (which would need to include the registration code as well as add and delete).

That seems a little bit on the large side, when really I just want to unit test one small function in LoopRooms.

> ::: browser/components/loop/test/mochitest/test_loopLinkClicker_channel.html
> @@ +27,5 @@
> > +  var event = new window.CustomEvent("WebChannelMessageToChrome", {
> > +    detail: {
> > +      id: "loop-link-clicker",
> > +      message: {
> > +        command: "checkWillOpenRoom",
> 
> These two files are identical, except for this command part. Isn't it
> possible to pass the command to send back as a query param?

Nice idea.
Updated for comments. AFAICT changing BrowserTestUtils won't break anything, but I'll run it through try as well (the main reason I hadn't used it was I hadn't spotted it up until now).
Attachment #8662271 - Attachment is obsolete: true
Attachment #8662912 - Flags: review?(mdeboer)
Comment on attachment 8662912 [details] [diff] [review]
Add a basic service channel implementation for listening to Loop's link clicker and opening rooms when requested.

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

Good stuff!

::: browser/components/loop/modules/LoopRooms.jsm
@@ +1115,5 @@
> +      for (let [key, value] of roomsCache) {
> +        LoopRoomsInternal.rooms.set(key, value);
> +      }
> +    }
> +  }

Nice, thanks! Another thing you could have done:

```js
let gRoomsMap = new Map();

let LoopRoomsInternal = {
  rooms: gRoomsMap
};

_setRoomsCache = function(newCache) {
  gRoomsMap = newCache;
};
```

Your call which one you prefer in the end! The advantage of changing the reference to a different Map instance altogether is that you can control it from within the unit test.

::: browser/components/loop/test/mochitest/browser_LoopRooms_channel.js
@@ +7,5 @@
> + * window.
> + */
> +"use strict";
> +
> +var {WebChannel} = Cu.import("resource://gre/modules/WebChannel.jsm", {});

nit: please use `let`.

@@ +20,5 @@
> +const LINKCLICKER_URL_PREFNAME = "loop.linkClicker.url";
> +
> +var openChatOrig = Chat.open;
> +
> +var fakeRoomList = new Map([[ ROOM_TOKEN, { roomToken: ROOM_TOKEN } ]]);

nit: `let` would work for these too, I believe.

::: browser/components/loop/test/mochitest/test_loopLinkClicker_channel.html
@@ +29,5 @@
> +  var event = new window.CustomEvent("WebChannelMessageToChrome", {
> +    detail: {
> +      id: "loop-link-clicker",
> +      message: {
> +        command: hash.substring(1, hash.length),

\o/

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +59,2 @@
>      options.gBrowser.removeTab(tab);
> +    return Promise.resolve(result);

Hmm, this looks more consistent indeed! I hope the try run turns green.
Attachment #8662912 - Flags: review?(mdeboer) → review+
Note: we're not handling the let comments for the global scope due to recent changes with how these are working.
The test broke on try for e10s, this is a new patch that I'm trying with a minor change to how the setup for the test channel is done.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c4442a863aa
Attachment #8662912 - Attachment is obsolete: true
Attachment #8662988 - Flags: review+
Depends on: 1206457
https://hg.mozilla.org/mozilla-central/rev/42b667704d78
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: