1066502 - Remove Backbone router from desktop panel and conversation window

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mike de Boer [:mikedeboer]]

Since we're not going to use URLs to navigate different sections and states in the panel and conversation window, it's no longer necessary to have a dependency to Backbone in desktop code.

It's safe to remove it. In fact, Niko` seemed to recall that navigation history manipulation for about:x urls does not work as expected, thus already needed to be worked around for the conversation window.

Additionally, not relying on the Backbone router code would easy the transition to eventually move the standalone app out of m-c into its own repo.

Comment 1

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #0)
> Since we're not going to use URLs to navigate different sections and states
> in the panel and conversation window, 

What made/how did we come to that decision?

I don't object to it (I never did like the different urls), but it'd be good just for a bit of explanation in case we want to refer later.

> it's no longer necessary to have a dependency to Backbone in desktop code.

I assume you mean just the Backbone router here for now (I think models & collections would need a different bug).

> It's safe to remove it. In fact, Niko` seemed to recall that navigation
> history manipulation for about:x urls does not work as expected, thus
> already needed to be worked around for the conversation window.

That's correct. Background:

http://hg.mozilla.org/mozilla-central/annotate/2db5b64f6d49/browser/components/loop/content/js/desktopRouter.js#l13

> Additionally, not relying on the Backbone router code would easy the
> transition to eventually move the standalone app out of m-c into its own
> repo.

I don't understand why dependencies on the backbone router would prevent moving out. I'm not saying that we can't remove it, but I don't believe there is a link here.

Should this bug be a meta as it is going to involve several parts, especially for the conversation windows?

Comment 2

[Mark Banner (:standard8)]

Bug 1066567 is likely to be doing some/all of the conversation side of this - certainly for standalone code.

Comment 3

[Mark Banner (:standard8)]

Attachment: Remove the backbone router from the Loop conversation window, use a react view for control

This removes the remaining router code from the conversation window. This was done in pretty much the same style as the webapp router removal.

This is needed so that we can start implementing direct calling in the conversation window on desktop.

Comment 4

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

Comment on attachment 8494353 [details] [diff] [review]
Remove the backbone router from the Loop conversation window, use a react view for control

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

Looks so much better, thanks! r+ with comments addressed.

::: browser/components/loop/content/js/conversation.jsx
@@ +241,5 @@
> +          document.title = mozL10n.get("incoming_call_title2");
> +
> +          // XXX Don't render anything initially, though this should probably
> +          // be some sort of pending view, whilst we connect the websocket.
> +          return (<div />);

Nit: null is usually preffered if you don't want anything to be rendered at all.

@@ +247,5 @@
> +        case "incoming": {
> +          document.title = mozL10n.get("incoming_call_title2");
> +
> +          return (
> +            <loop.conversation.IncomingCallView

<IncomingCallView /> is good enough.

@@ +258,5 @@
> +         // XXX This should be the caller id (bug 1020449)
> +         document.title = mozL10n.get("incoming_call_title2");
> +
> +         var callType = this.props.conversation.get("selectedCallType");
> +          var videoStream = callType === "audio" ? false : true;

var videoStream = callType !== "audio"; is sufficient. Also, little indentation pb, and read comment below as I think we could just compute that boolean inline in JSX.

@@ +265,5 @@
> +            <sharedViews.ConversationView
> +              initiate={true}
> +              sdk={this.props.sdk}
> +              model={this.props.conversation}
> +              video={{enabled: videoStream}}

video={{enabled: callType !== "audio"}} or even video={{enabled: callType === "audio-video"}} are probably good enough.

@@ +293,5 @@
> +          );
> +        }
> +        case "close": {
> +          window.close();
> +          return (<div/>);

Huh :-)

As you can see, the FeedbackView onAfterFeedbackReceived props does that already, I think we could share a single method for closing the window.

Also, I don't think we actually need a "close" state, but maybe this is on purpose to ease implementation of a future feature?

@@ +305,3 @@
>       */
> +    _notifyError: function(error) {
> +      console.log(error);

Nit: console.error

@@ +491,5 @@
>      mozL10n.initialize(navigator.mozLoop);
>  
>      // Plug in an alternate client ID mechanism, as localStorage and cookies
>      // don't work in the conversation window
> +    window.OT.overrideGuidStorage({

Any specific reason we removed the check for OT being available here?

@@ -456,5 @@
>      mozL10n.initialize(navigator.mozLoop);
>  
>      // Plug in an alternate client ID mechanism, as localStorage and cookies
>      // don't work in the conversation window
> -    if (OT && OT.hasOwnProperty("overrideGuidStorage")) {

Any specific reason we removed the check for OT being available here?

@@ +519,5 @@
> +    // Obtain the callId and pass it to the conversation
> +    var helper = new loop.shared.utils.Helper();
> +    var locationHash = helper.locationHash();
> +    if (locationHash) {
> +      conversation.set("callId", locationHash.match(/\#incoming\/(.*)/)[1]);

This probably won't happen much, but if locationHash.match() returns null this will break.

@@ +520,5 @@
> +    var helper = new loop.shared.utils.Helper();
> +    var locationHash = helper.locationHash();
> +    if (locationHash) {
> +      conversation.set("callId", locationHash.match(/\#incoming\/(.*)/)[1]);
> +     }

Nit: indentation.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +138,5 @@
> +    describe("start", function() {
> +      it("should set the title to incoming_call_title2", function() {
> +        icView = mountTestComponent();
> +
> +         expect(document.title).eql("fakeText");

Nit: indentation. Also I'd expect the fake string being defined locally for that test, to ensure it's the one we're actually using.

@@ +489,4 @@
>          conversation.set("loopToken", "fakeToken");
> +        navigator.mozLoop.getLoopCharPref.returns("http://fake");
> +        stubComponent(sharedView, "ConversationView");
> +//        stubComponent(sharedView, "FeedbackView");

Could be entirely removed.

Comment 5

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #4)
>> @@ -456,5 @@
> >      mozL10n.initialize(navigator.mozLoop);
> >  
> >      // Plug in an alternate client ID mechanism, as localStorage and cookies
> >      // don't work in the conversation window
> > -    if (OT && OT.hasOwnProperty("overrideGuidStorage")) {
> 
> Any specific reason we removed the check for OT being available here?

There's no need for it, now I've refactored, I've been able to implement proper stubbing.

Comment 6

[Mark Banner (:standard8)]

Attachment: Remove the backbone router from the Loop conversation window, use a react view for control.

Updated patch fixing comments.

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

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

Comment on attachment 8494496 [details] [diff] [review]
Remove the backbone router from the Loop conversation window, use a react view for control.

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/715696027179

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8494496 [details] [diff] [review]
Remove the backbone router from the Loop conversation window, use a react view for control.

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.