1106474 - Drop conversationStore's use of Backbone.Model

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

As part of our improvements to flux, we implemented a new loop.store.createStore to remove some of the boilerplate code that we've got in our stores.

conversationStore hasn't switched to it yet, now seems like a good time to do that, especially before bug 1088672.

Comment 1

[Mark Banner (:standard8)]

Attachment: Drop conversationStore's use of Backbone.Model for Loop.

Most of this is adapting g/set to g/setStoreState.

Comment 2

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

Comment on attachment 8530743 [details] [diff] [review]
Drop conversationStore's use of Backbone.Model for Loop.

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

Looks good, some possible dead code to be removed for even more cleanup though :)

::: browser/components/loop/content/js/conversationViews.jsx
@@ +456,1 @@
>        }, this);

Note: Can't wait to have a StoreMixin to avoid duplicating this boilerplate every time… Please ensure a bug is filed about it.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +66,5 @@
> +  loop.store.ConversationStore = loop.store.createStore({
> +    // XXX Further actions are registered in setupWindowData when
> +    // we know what window type this is. At some stage, we might want to
> +    // consider store mixins or some alternative which means the stores
> +    // would only be created when we want them.

Same remark as above about checking that a bug is already filed for this.

@@ +80,5 @@
> +        callState: CALL_STATES.INIT,
> +        // The reason if a call was terminated
> +        callStateReason: undefined,
> +        // The error information, if there was a failure
> +        error: undefined,

This isn't used and should be removed.

@@ +93,5 @@
> +        // Call Connection information
> +        // The call id from the loop-server
> +        callId: undefined,
> +        // The caller id of the contacting side
> +        callerId: undefined,

This seems not to be used anymore, can you check?

@@ +108,5 @@
> +        // If the audio is muted
> +        audioMuted: false,
> +        // If the video is muted
> +        videoMuted: false
> +      };

Could you please add a default for `emailLink`?

@@ +172,5 @@
>          case WS_STATES.CONNECTING: {
>            this.sdkDriver.connectSession({
> +            apiKey: this.getStoreState("apiKey"),
> +            sessionId: this.getStoreState("sessionId"),
> +            sessionToken: this.getStoreState("sessionToken")

Nit: Instead of calling getStoreState for each key, you could store the whole state object in a var before the switch statement, and reference its properties a little less verbosely.

@@ +350,2 @@
>  
> +      this.mozLoop.calls.setCallInProgress(this.getStoreState("windowId"));

Much nicer :)

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +246,2 @@
>          client: {},
> +        mozLoop: navigator.mozLoop,

Can't we create a fakeMozLoop object like you've done in conversationStore_test.js?

Comment 3

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #2)
> @@ +93,5 @@
> > +        // Call Connection information
> > +        // The call id from the loop-server
> > +        callId: undefined,
> > +        // The caller id of the contacting side
> > +        callerId: undefined,
> 
> This seems not to be used anymore, can you check?

It'll be used in the next patch.

> ::: browser/components/loop/test/desktop-local/conversationViews_test.js
> @@ +246,2 @@
> >          client: {},
> > +        mozLoop: navigator.mozLoop,
> 
> Can't we create a fakeMozLoop object like you've done in
> conversationStore_test.js?

Changed it for this particular instance, filed bug 1106857 on fixing the rest.

Comment 4

[Mark Banner (:standard8)]

Attachment: Drop conversationStore's use of Backbone.Model for Loop.

Updated for review comments.

Comment 5

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

Comment on attachment 8531314 [details] [diff] [review]
Drop conversationStore's use of Backbone.Model for Loop.

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

LGTM, ship it!

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/374e01ce7430