If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Drop conversationStore's use of Backbone.Model

RESOLVED FIXED in mozilla37

Status

Hello (Loop)
Client
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla37
Points:
2

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8530743 [details] [diff] [review]
Drop conversationStore's use of Backbone.Model for Loop.

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-
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
Created attachment 8531314 [details] [diff] [review]
Drop conversationStore's use of Backbone.Model for Loop.

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+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/374e01ce7430
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/374e01ce7430
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.