1097703 - Standalone version of UI for ROOM clickers should allow users to install and/or open the FxOS Loop client

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Maria Angeles Oteo (:oteo)]

The hosted Loop client should check for the FxOS Loop client and allow the user to install it if it is not already installed in the device or to open it to handle the connection to the ROOM if it is already installed.

Basically, it's the same US than the one in bug 1008990 but for Rooms

Comment 1

[Maire Reavy [:mreavy]]

Maria -- What is your expected timeline for implementing Rooms on the Hello app?  

On Bug 1008990, a developer from your team implemented the changes you wanted on the standalone app, and a developer from my team reviewed it.  I'd like to do something similar for this bug.

Comment 2

[Maria Angeles Oteo (:oteo)]

We want to have landed all the Rooms features at the end of December but we need the Standalone UI working earlier otherwise we can not test properly.

Comment 3

[Carmen Jimenez Cabezas]

Attachment: V1 WIP

Comment 4

[Carmen Jimenez Cabezas]

Attachment: V1 WIP

Comment 5

[Maria Angeles Oteo (:oteo)]

Maire, who developer from your team can help us with the feedback/review process? Carmen has WIP patch that would like to someone to have a look at it?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8527378 [details] [diff] [review]
V1 WIP

I can take a look at this. It'll likely be tomorrow or Wednesday.

Comment 7

[Carmen Jimenez Cabezas]

Attachment: v1 WIP

I've only rebased the old wip

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8528293 [details] [diff] [review]
v1 WIP

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

Sorry for the delay in getting to this. The general idea isn't too bad, but there's a couple of things I'm concerned about:

- The way the views and stores interact doesn't quite fit the flux style we've been dealing with. I've added comments inline so you can see the changes that should be necessary.

- The new FxOSRoomStore duplicates a lot of the existing activeRoomStore. That's a bit worrying from a maintainability aspect. I think we should look at either some sort of mixin or some other way of removing the duplication.

If you want to chat more about this, or have more thoughts, I'm happy to help.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +25,5 @@
>      // The room is full
>      FULL: "room-full"
>    };
>  
> +loop.store.FxOSRoomStore = (function() {

I would go with FxOSActiveRoomStore. I think we might want to make it a new file, but lets see how we can refine it first.

::: browser/components/loop/standalone/content/js/fxOSMarketplace.jsx
@@ +22,5 @@
> +      if (this.props.onMarketplaceMessage) {
> +        // The reason for listening on the global window instead of on the
> +        // iframe content window is because the Marketplace is doing a
> +        // window.top.postMessage.
> +        window.addEventListener("message", this.props.onMarketplaceMessage);

The callback from the event listener should dispatch an action with the necessary data to be handled.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +238,5 @@
>      },
>  
> +    _onFxOSAppNeeded: function(onMarketplaceMessageHandler) {
> +      this.setState({
> +        marketplaceSrc: loop.config.marketplaceUrl,

FxOSRoomStore should be setting this in its store state - the view will then pick it up via the listeners.

@@ +247,3 @@
>      joinRoom: function() {
> +      this.props.dispatcher.dispatch(new sharedActions.JoinRoom({
> +        fireAppInstallation: this._onFxOSAppNeeded.bind(this)

This shouldn't be necessary with the changes I've mentioned elsewhere.

Comment 9

[Carmen Jimenez Cabezas]

Attachment: V2 wip

Comment 10

[Carmen Jimenez Cabezas]

Comment on attachment 8530808 [details]
V2 wip

New version. There are still some unit tests missing here. I think I addressed all your comments except:

>The callback from the event listener should dispatch an action with the
>necessary data to be handled.

The code you commented is common between the standalone conversation UI and the standalone room UI, and the conversation UI doesn't have the concept of "actions". So changing that handler to a new WhateverAction(actionData) is not really an option there.

Comment 11

[Carmen Jimenez Cabezas]

Attachment: V2 proposed patch

Comment 12

[Carmen Jimenez Cabezas]

Attachment: V2 proposed patch

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8531223 [details] [diff] [review]
V2  proposed patch

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

Its looking a lot clearer with this update. Though there's still a few things to address.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +29,5 @@
>      // The room conversation has ended
>      ENDED: "room-ended"
>    };
>  
> +loop.store.FxOSActiveRoomStore = (function() {

Please can you make this store into a separate file. Now its reduced in size and easier to understand, I think it makes sense given how we've split our files.

@@ +63,5 @@
> +     * Handles a room failure.
> +     *
> +     * @param {sharedActions.RoomFailure} actionData
> +     */
> +    roomFailure: function(actionData) {

This function can be dropped, since it isn't used at the moment.

@@ +103,5 @@
> +
> +    /**
> +     * Handles the action to join to a room.
> +     */
> +    joinRoom: function(actionData) {

nit: no need to specify actionData here.

@@ +112,5 @@
> +
> +      this.setupOutgoingRoom(true);
> +    },
> +
> +    setupOutgoingRoom: function(installApp) {

Please make this an underscore function (_setupOutgoingRoom) to indicate that its private, and please add a jsdoc for the function.

@@ +142,5 @@
> +        });
> +      }).bind(this);
> +    },
> +
> +    _onMarketplaceMessage: function(event) {

nit: please add jsdoc describing the parameters and what this is for and how it gets called.

@@ +332,5 @@
>            }));
>  
>            // For the conversation window, we need to automatically
>            // join the room.
> +          this.dispatchAction(new sharedActions.JoinRoom({}));

nit: no need for the braces addition

::: browser/components/loop/standalone/content/js/fxOSMarketplace.jsx
@@ +22,5 @@
> +      if (this.props.onMarketplaceMessage) {
> +        // The reason for listening on the global window instead of on the
> +        // iframe content window is because the Marketplace is doing a
> +        // window.top.postMessage.
> +        window.addEventListener("message", this.props.onMarketplaceMessage);

Please add an XXX comment in here to say that this should be changed to an action once the old-style call urls go away.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +309,5 @@
>        }
>      },
>  
>      joinRoom: function() {
> +      this.props.dispatcher.dispatch(new sharedActions.JoinRoom({}));

nit: no need to add the double braces in here

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +1036,5 @@
>        sdk: OT
>      });
> +    var conversation;
> +    var activeRoomStore;
> +    if (helper.isFirefoxOS(navigator.userAgent)) {

Please can we make the logic here slightly different. I want to make it so that we are able to not enable the rooms part of this on production until it has been fully tested.

Additionally, it wouldn't hurt to have the existing conversation code switched on if the fxosApp is defined or not.

Something like:

if (helper.isFirefoxOS(...) {
  if (loop.config.fxosApp) {
    // Use the FxOSConversationModel
    if (loop.config.fxosApp.rooms) {
      // Use the FxOSActiveRoomStore
    }
  }
}

// If none of the above apply, 
//   Use the ConversationModel
//   Use the ActiveRoomStore

loop.config.fxosApp.rooms would be a new configuration value - please add it to Makefile and to server.js.

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +217,5 @@
>          }));
>  
>          sinon.assert.calledTwice(dispatcher.dispatch);
>          sinon.assert.calledWithExactly(dispatcher.dispatch,
> +          new sharedActions.JoinRoom({}));

nit: there's a few places of added braces in this file that can be dropped.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +229,5 @@
>            TestUtils.Simulate.click(getJoinButton(view));
>  
>            sinon.assert.calledOnce(dispatch);
> +          sinon.assert.calledWithExactly(dispatch,
> +                                         new sharedActions.JoinRoom({}));

nit: please remove this change.

Comment 14

[Carmen Jimenez Cabezas]

Attachment: V3 proposed patch

Comment 15

[Carmen Jimenez Cabezas]

Attachment: V3 proposed patch

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8534931 [details] [diff] [review]
V3  proposed patch

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

Sorry for the delay, looks good. r=Standard8

Comment 17

[Carmen Jimenez Cabezas]

Attachment: V3 proposed patch rebased

r = standard8

Comment 18

[Carmen Jimenez Cabezas]

try:
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ef4bce3c7b3

Bug 1109032 must land before this one (otherwise, this patch won't apply).

Comment 19

[Maria Angeles Oteo (:oteo)]

We need this for Fx35

Comment 20

[Maire Reavy [:mreavy]]

This is for the standalone app -- so it just needs to land and merge in m-c this week (which is officially the Fx37 cycle), and it will be in the next release of the standalone app.  We are planning to do another standalone release before Fx35 goes to release (Jan 13).  In bug 1109032 I asked "how soon do you need the standalone app updated?"

Comment 21

[Maria Angeles Oteo (:oteo)]

Already answered in bug 1109032

Comment 22

[Carmen Jimenez Cabezas]

treeherder looks good.
Requesting checkin

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af406c83a06e