Remove Backbone router from desktop panel and conversation window

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mikedeboer, Assigned: standard8)

Tracking

unspecified
mozilla35
Points:
2
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

(Whiteboard: [loop-uplift])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
(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?
Assignee

Comment 2

5 years ago
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-]
Assignee

Updated

5 years ago
Depends on: 1068580
Assignee

Comment 3

5 years ago
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

Updated

5 years ago
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]
Assignee

Comment 5

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

Comment 6

5 years ago
Updated patch fixing comments.
Attachment #8494353 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a48a344fb852
Status: NEW → RESOLVED
Closed: 5 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.