1090209 - Opening conversation windows shouldn't need an id in the url

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

When we open a conversation window, we shouldn't need to include an id for the window in the url. This introduces unnecessary complexity:

- It requires the addition of temporary call id in some cases (e.g. outgoing calls).
- The call ids must not clash (otherwise you get issues like bug 1086434)

Although opening a conversation window without getting an id might cause an issue if opening multiple windows at the same time, worst-case we should be able to queue expected windows and have the details taken from the queue at the appropriate time.

Additionally, we should also not need to include the type of call/room in the url - this can be simply obtained from mozLoop along with the rest of the call/room data.

We might need some special management for the call busy statuses, but I'd rather not have to do id management to achieve this.

Comment 1

[Mark Banner (:standard8)]

In commenting on bug 1086434, I've just remembered we do need something different in the url, as otherwise we'll only be able to have one window opened.

This could be an id, although I'd be more tempted to do an incrementing number, that's not directly associated to call id or room id.

This would save the confusion of ids that we have currently, and ensure windows can always be opened successfully.

Comment 2

[Mark Banner (:standard8)]

Attachment: Drop the window type from the about:loopconversation url.

This drops the window type from the url that opens a Loop conversation window, and passes it in the call data instead.

We then create an appStore for responsible for getting the appropriate data, storing the type and passing the data onto other stores. The store is then also used to drive the AppControllerView in selecting which view to display.

This takes the complexity out of init() and makes the top-level view selection much easier to follow.

Comment 3

[Mark Banner (:standard8)]

Oh, I forgot to mention - I may do a part 2 which would be to change how we manage the call data and where that's obtained from, but I'm still debating that.

Comment 4

[Mark Banner (:standard8)]

Attachment: WIP Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

This is a WIP to manage the window ids centrally and convert it to an incrementing number to remove any chance of conflicts. Additionally, we then revise LoopCalls way of handling busy statuses such that it only handles the busy state and nothing else.

The patch works, but I need to update tests next.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8513603 [details] [diff] [review]
Drop the window type from the about:loopconversation url.

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

LGTM with comments addressed, as per reviewed over Vidyo.

::: browser/components/loop/LoopCalls.jsm
@@ +301,2 @@
>        // XXX Really we shouldn't be using random numbers, bug 1090209 will fix this.
>        windowId: Math.floor((Math.random() * 100000000))

Possible id conflict, but it will be addressed in part 2.

::: browser/components/loop/content/js/appStore.js
@@ +10,5 @@
> +/**
> + * Manages the conversation window app controller view. Used to get
> + * the window data and store the window type.
> + */
> +loop.store.AppStore = (function() {

How about ConversationAppStore?

@@ +57,5 @@
> +    /**
> +     * Handles the get window data action - obtains the window data,
> +     * updates the store and notifies interested components.
> +     *
> +     * @param {sharedActions.GetWindowData} actionData The action data for the action.

Nit: s/The action data for the action./The action data.

@@ +65,5 @@
> +      // XXX Remove me in bug 1074678
> +      if (this._mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
> +        windowData = {type: "room", localRoomId: "42"};
> +      } else {
> +        windowData = this._mozLoop.getCallData(actionData.id);

s/id/windowId?

::: browser/components/loop/content/js/conversation.jsx
@@ +183,5 @@
>     *
>     * XXX Based on CallFailedView, but built specially until we flux-ify the
>     * incoming call views (bug 1088672).
>     */
> +  var SomethingWentWrongView = React.createClass({

How about GenericFailureView?

@@ +564,5 @@
> +        return null;
> +      }
> +
> +
> +      if (this.state.windowType === "failed") {

Nit: How about using a switch statement, like we do elsewhere?

@@ +651,2 @@
>  
> +    var hash = locationHash.match(/#(.*)/);

Nit: /#(\d+)$/

@@ +671,5 @@
>        sdk={window.OT}
>      />, document.querySelector('#main'));
>  
> +    dispatcher.dispatch(new sharedActions.GetWindowData({
> +      id: windowId

I'm wondering if we shouldn't just avoid dispatching if we have no windowId.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +107,5 @@
>  
>      render: function() {
>        var contactName;
>  
> +      if (this.props.contact) {

How could this be happening?

@@ +118,3 @@
>        }
>  
>        document.title = contactName;

Won't this show "undefined" if no props.contact is passed?

::: browser/components/loop/content/shared/js/actions.js
@@ +33,5 @@
>      /**
> +     * Get the window data for the provided window id
> +     */
> +    GetWindowData: Action.define("getWindowData", {
> +      id: String

Nit: How about keeping `windowID` as a descriptor for window ids?

@@ +41,5 @@
> +     * Used to pass round the window data so that stores can
> +     * record the appropriate data.
> +     */
> +    SetupWindowData: Action.define("setupWindowData", {
> +      data: Object

How about `windowData`?

::: browser/components/loop/test/xpcshell/test_loopservice_directcall.js
@@ +25,5 @@
>  
>    do_check_true(!!openedUrl, "should open a chat window");
>  
>    // Stop the busy kicking in for following tests.
> +  let callId = openedUrl.match(/about:loopconversation\#(.*)/)[1];

#(\d/) ?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8513603 [details] [diff] [review]
Drop the window type from the about:loopconversation url.

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

::: browser/components/loop/content/js/conversation.jsx
@@ +651,2 @@
>  
> +    var hash = locationHash.match(/#(.*)/);

I tried this, but the incoming conversation window id is not yet numbers only, so it broke. Hence I undid it for now.

@@ +671,5 @@
>        sdk={window.OT}
>      />, document.querySelector('#main'));
>  
> +    dispatcher.dispatch(new sharedActions.GetWindowData({
> +      id: windowId

The dispatch will actually cause the failed view to be shown. Obtaining the windowId can probably move into conversationAppStore soon, once we do the flux rework for the incoming conversation views. So I think this will make more sense then.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +107,5 @@
>  
>      render: function() {
>        var contactName;
>  
> +      if (this.props.contact) {

Ok, I took a look and it and it was affecting the running client. The issue was that the way the actions are now structured, the AppControllerView was rendering the "outgoing" call, before the data had been obtained.

I've therefore done a change to make the OutgoingConversationView display nothing if the call state is "init". As soon as the data's been got from gecko it switches to "gather", so we'll then re-render and display something properly.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 1 Drop the window type from the url that opens a Loop conversation window, and pass it in the call data instead.

Updated patch for review comments.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8514243 [details] [diff] [review]
Part 1 Drop the window type from the url that opens a Loop conversation window, and pass it in the call data instead.

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

LGTM

::: browser/components/loop/content/js/conversationViews.jsx
@@ +500,5 @@
>            return this._renderFeedbackView();
>          }
> +        case CALL_STATES.INIT: {
> +          // We know what we are, but we haven't got the data yet.
> +          return null;

Smart.

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

This makes MozLoopService handle the ids centrally, storing the data until the window is opened. LoopCalls now handles busy itself - afaik there's no requirement at the current time for busy for rooms, if it comes later, then we can move it across to MozLoopService.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

Minor change to not spam a console warning when clearCallInProgress is called with no stored id (e.g. in the case of a room window having been opened - but we have a brute-force clearing of the call from the window id in place, just in case it hasn't already happened).

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8514337 [details] [diff] [review]
Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

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

Looking great! I do have a couple of suggestions that might make the whole thing even nicer, perhaps.
Therefore I'd like to see the final patch once more.

::: browser/components/loop/LoopCalls.jsm
@@ +126,5 @@
>      let msg = {};
>      try {
>        msg = JSON.parse(aMsg);
>      }
>      catch (error) {

nit: can you put the catch on the same line as the curly while you're here?

::: browser/components/loop/MozLoopAPI.jsm
@@ +222,5 @@
> +          targetWindow);
> +      }
> +    },
> +
> +    setCallInProgress: {

*sigh* I wish we'd just have a `mozLoop.calls` namespace and use injectObjectAPI for this...

::: browser/components/loop/MozLoopService.jsm
@@ +116,5 @@
>  let gFxAOAuthClientPromise = null;
>  let gFxAOAuthClient = null;
>  let gErrors = new Map();
> +let gLastWindowId = 0;
> +let gConversationWindowData = {};

Please make this a Map(), because you're using numbers as keys.

@@ +752,5 @@
>          pc_static.registerPeerConnectionLifecycleCallback(onPCLifecycleChange);
>        }.bind(this), true);
>      };
>  
> +    Chat.open(null, origin, "", url, undefined, undefined, callback);

TBH, I've never seen explicit passing of `undefined` as argument values... Why not use `null`?

@@ +1419,5 @@
>    hawkRequest: function(sessionType, path, method, payloadObj) {
>      return MozLoopServiceInternal.hawkRequest(sessionType, path, method, payloadObj).catch(
>        error => {MozLoopServiceInternal._hawkRequestError(error);});
>    },
> +

I'd appreciate a docblock comment here.

@@ +1423,5 @@
> +
> +  getConversationWindowData: function(conversationWindowId) {
> +    var conversationData = gConversationWindowData[conversationWindowId];
> +    delete gConversationWindowData[conversationWindowId];
> +    return conversationData;

So calling this a second time will yield an error? Can you add a guard for that, perhaps even throw an error with a message 'Window data was already fetched before. Possible race condition!'

::: browser/components/loop/content/js/conversationAppStore.js
@@ +65,5 @@
>        // XXX Remove me in bug 1074678
>        if (this._mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
>          windowData = {type: "room", localRoomId: "42"};
>        } else {
> +        windowData = this._mozLoop.getConversationWindowData(actionData.windowId);

\o/

@@ +79,5 @@
> +      // we rework it for the flux model in bug 1088672.
> +      this.setStoreState({
> +        windowType: windowData.type,
> +        windowData: windowData
> +      });

Can't we just do something like `_.extend({windowType: windowData.type}, windowData)`

I'd like to prevent having to keep track and making up names whilst we can live with a flat data object, don't you agree?

::: browser/components/loop/content/shared/js/actions.js
@@ +41,5 @@
>       * Used to pass round the window data so that stores can
>       * record the appropriate data.
>       */
>      SetupWindowData: Action.define("setupWindowData", {
> +      windowId: String,

With a flattened Object, you can also be more specific about what the action data ought to contain.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +199,5 @@
>  
>        this.set({
>          contact: windowData.contact,
>          outgoing: windowType === "outgoing",
> +        windowId: actionData.windowId,

The mix between `windowData` and `actionData` is a bit fuzzy to me... can you remind me?

::: browser/components/loop/test/shared/localRoomStore_test.js
@@ +63,4 @@
>          windowData: {
>            type: "room",
>            localRoomId: fakeRoomId
>          }

So, with a flattened object, this would become:

```js
new sharedActions.SetupWindowData({
  id: "42",
  type: "room",
  localRoomId: fakeRoomId
});
```

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8514337 [details] [diff] [review]
Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +222,5 @@
> +          targetWindow);
> +      }
> +    },
> +
> +    setCallInProgress: {

Ok, I'll bite. I'm touching 3/4 of the existing interface anyway...

::: browser/components/loop/MozLoopService.jsm
@@ +752,5 @@
>          pc_static.registerPeerConnectionLifecycleCallback(onPCLifecycleChange);
>        }.bind(this), true);
>      };
>  
> +    Chat.open(null, origin, "", url, undefined, undefined, callback);

I checked, and it needs to be undefined:

http://hg.mozilla.org/mozilla-central/annotate/e0b505a37b1c/browser/modules/Chat.jsm#l93

::: browser/components/loop/content/js/conversationAppStore.js
@@ +79,5 @@
> +      // we rework it for the flux model in bug 1088672.
> +      this.setStoreState({
> +        windowType: windowData.type,
> +        windowData: windowData
> +      });

I'd prefer not to do that for this store. The windowData is a temporary item until we fix 1088672. When we fix that, we'll only need to store the windowType, so flattening it here for the store itself doesn't seem to add enough value - it does for the action though.

Comment 13

[Mark Banner (:standard8)]

Attachment: Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

Updated patch for review comments.

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8515069 [details] [diff] [review]
Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

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

r=me with the one comment below addressed - this'll make it so that we support return values for API functions. If a function throws, the consumer still needs to check for an `Error` return value, but that's probably not a big deal... or if that's too hacky in your opinion, you can leave the try...catch out.

I'd recommend a push to Try before landing this.

Nice work!

::: browser/components/loop/MozLoopAPI.jsm
@@ +112,5 @@
> +        api[func](...params, function(...results) {
> +          lastParam(...[cloneValueInto(r, targetWindow) for (r of results)]);
> +        });
> +      } else {
> +        api[func](...params, lastParam);

You'll want to make this:

```js
try {
  return cloneValueInto(api[func](...params, lastParam), targetWindow);
} catch (ex) {
  return cloneValueInto(ex, targetWindow);
}
```

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 1 Drop the window type from the url that opens a Loop conversation window, and pass it in the call data instead.

Updated for bitrot.

Comment 16

[Mark Banner (:standard8)]

Attachment: Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

Updated to address review comments and bitrot

Comment 17

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/13cdd8fb2576
https://hg.mozilla.org/integration/fx-team/rev/f72b4136675c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13cdd8fb2576
https://hg.mozilla.org/mozilla-central/rev/f72b4136675c

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fd86797e66d
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa172bfb0bfc

Comment 20

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

Comment on attachment 8515956 [details] [diff] [review]
Part 1 Drop the window type from the url that opens a Loop conversation window, and pass it in the call data instead.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 21

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

Comment on attachment 8515957 [details] [diff] [review]
Part 2 Use MozLoopService to manage window ids centrally, and store the data for the window opening.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only