Closed Bug 1066502 Opened 6 years ago Closed 6 years ago

Remove Backbone router from desktop panel and conversation window

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
2

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: mikedeboer, Assigned: standard8)

References

Details

(Whiteboard: [loop-uplift])

Attachments

(1 file, 1 obsolete file)

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.
(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?
Bug 1066567 is likely to be doing some/all of the conversation side of this - certainly for standalone code.
Depends on: 1066567
Flags: qe-verify-
Whiteboard: [qa-]
Depends on: 1068580
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.
Attachment #8494353 - Flags: review?(nperriault)
Assignee: nobody → standard8
Blocks: 1066567
No longer depends on: 1066567
Target Milestone: --- → mozilla35
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.
Attachment #8494353 - Flags: review?(nperriault) → review+
Whiteboard: [loop-uplift]
(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.
Updated patch fixing comments.
Attachment #8494353 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a48a344fb852
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #8494496 - Flags: approval-mozilla-aurora?
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.
Attachment #8494496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.