Closed Bug 1106474 Opened 5 years ago Closed 5 years ago

Drop conversationStore's use of Backbone.Model

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
2

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37
Iteration:
37.1

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [tech-debt])

Attachments

(1 file, 1 obsolete file)

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.
Most of this is adapting g/set to g/setStoreState.
Attachment #8530743 - Flags: review?(nperriault)
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?
Attachment #8530743 - Flags: review?(nperriault) → review-
(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.
Updated for review comments.
Attachment #8530743 - Attachment is obsolete: true
Attachment #8531314 - Flags: review?(nperriault)
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!
Attachment #8531314 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/integration/fx-team/rev/374e01ce7430
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/374e01ce7430
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.