Desktop client needs call initiation to Contacts for logged in FxA users

VERIFIED FIXED in Firefox 34

Status

Hello (Loop)
Client
P3
enhancement
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: abr, Assigned: standard8)

Tracking

unspecified
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap +
qe-verify +

Firefox Tracking Flags

(firefox34 verified, firefox35 verified)

Details

(Whiteboard: [p=5, contacts, fxa, first release needed][loop-uplift][loop-outcall1])

User Story

As a signed-in FF browser user I can call a FxA contact.  We should file a separate bug to allow a user to choose to call via Audio only or Audio+Video so that the user can define if the callee will see the user or not.

Attachments

(4 attachments, 4 obsolete attachments)

41.32 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
72.84 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
62.23 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
64.10 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

4 years ago
Blocks: 972015
(Reporter)

Updated

4 years ago
No longer blocks: 972015
(Reporter)

Updated

4 years ago
Blocks: 972014
(Assignee)

Updated

4 years ago
Blocks: 972866
(Assignee)

Updated

4 years ago
No longer blocks: 972866
(Assignee)

Updated

4 years ago
Component: General → Client

Updated

4 years ago
User Story: (updated)

Updated

4 years ago
Depends on: 1000178

Updated

4 years ago
Depends on: 1000183

Updated

4 years ago
Depends on: 1000186

Updated

4 years ago
Depends on: 1000197

Updated

4 years ago
Depends on: 1000769

Updated

4 years ago
No longer depends on: 1000178

Updated

4 years ago
No longer depends on: 1000183

Updated

4 years ago
No longer depends on: 1000186

Updated

4 years ago
No longer depends on: 1000197

Updated

4 years ago
No longer depends on: 1000769

Updated

4 years ago
Priority: -- → P1
Whiteboard: [s=ui32]
Target Milestone: --- → mozilla32

Updated

4 years ago
Blocks: 1012743

Updated

4 years ago
No longer blocks: 972014

Updated

4 years ago
Whiteboard: [s=ui32] → p=?
Target Milestone: mozilla32 → mozilla33

Updated

4 years ago
Summary: Desktop client needs call initiation UI → Desktop client needs call initiation

Comment 1

4 years ago
If a contact holds multiple identities (e-mail addresses and phone numbers), these multiple identities are sent to the server which will try to connect identities which are registered and connected.
If multiple identities are registered they are all notified though push notifications and the first identity which answers or declines causes all other engaged identities to have their incoming calls cancelled (first who answers or declines wins).

Updated

4 years ago
Depends on: 1015085

Updated

4 years ago
Blocks: 1015491

Updated

4 years ago
No longer depends on: 1015085
Multiple destinations seems quite separable from this piece, and notably lower priority than just placing a call to a contact at all, so I've split that out to bug 1015491.

Updated

4 years ago
Priority: P1 → P3

Updated

4 years ago
Priority: P3 → P1
Target Milestone: mozilla33 → mozilla34

Updated

3 years ago
Summary: Desktop client needs call initiation → Desktop client needs call initiation for logged in FxA users

Comment 3

3 years ago
tagged with contacts, depends on contacts & FxA bug, removed priority as it's blocked on 2 fronts now.
Depends on: 972015, 979845
Priority: P1 → --
Whiteboard: p=? → [p=?, contacts, fxa]

Updated

3 years ago
Summary: Desktop client needs call initiation for logged in FxA users → Desktop client needs call initiation to Contacts for logged in FxA users
Priority: -- → P2

Updated

3 years ago
Target Milestone: mozilla34 → mozilla35
I don't think this is needed for the first release of FxA & contacts.
I assume this user story is to support the choice of audio only versus audio plus video (which is the default).  The ability to initiate a video call for a signed in FxA desktop user is a different bug.

Comment 6

3 years ago
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #4)
> I don't think this is needed for the first release of FxA & contacts.

I disagree. 55% of Skype calls are audio only despite the fact that users have the option to use Video.
I think a fair share of calls won't happen if we force the use of Video to users.
A lot of people don't want to show their video - I think this is necessary for GA.

Comment 7

3 years ago
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> I assume this user story is to support the choice of audio only versus audio
> plus video (which is the default).  The ability to initiate a video call for
> a signed in FxA desktop user is a different bug.

This bug is for call initiation from the contact list (direct calling) using audio only or audio+video. I can't see any other bug related to the "initiation of a video call for a signed in FxA desktop user" so I suggest we break this bug down for "audio only and "audio+video" call initiation if it makes technical sense.

Updated

3 years ago
Whiteboard: [p=?, contacts, fxa] → [p=?, contacts, fxa, first release needed]

Updated

3 years ago
Depends on: 1046580
We should file a follow up bug to support audio only calling for signed in users.
User Story: (updated)
Points: --- → 5

Updated

3 years ago
Whiteboard: [p=?, contacts, fxa, first release needed] → [p=5, contacts, fxa, first release needed]

Updated

3 years ago
Priority: P2 → P3
Target Milestone: mozilla35 → mozilla34

Updated

3 years ago
Flags: firefox-backlog+

Updated

3 years ago
Whiteboard: [p=5, contacts, fxa, first release needed] → [p=5, contacts, fxa, first release needed][loop-uplift]
Target Milestone: mozilla34 → mozilla35
(Assignee)

Updated

3 years ago
Blocks: 1066017
No longer blocks: 1012743
Severity: normal → enhancement
(Assignee)

Updated

3 years ago
Depends on: 1066567
(Assignee)

Updated

3 years ago
Depends on: 1066568
(Assignee)

Updated

3 years ago
Whiteboard: [p=5, contacts, fxa, first release needed][loop-uplift] → [p=5, contacts, fxa, first release needed][loop-uplift][loop-outcall1]
(Assignee)

Updated

3 years ago
Depends on: 1072323
(Assignee)

Comment 9

3 years ago
Created attachment 8495232 [details] [diff] [review]
Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

This sets up two controller views for the conversation window - an overall controller, which selects between outgoing and incoming, and then an outgoing conversation controller.

In addition, there's a bare-bones pending view for the outgoing conversation I've added. Its intended that the sub-views be stateless, but we'll have to see how pratical that is as we go.

For now, I've set up a hidden pref ("loop.outgoingemail") to work around bug 1072323. Setting this pref to an email address, and then "receiving" an incoming call, will trigger the window to open as if it was an outgoing call.

Once we fix the bug, we can remove the pref as appropraite.

Next step is to get some of the action/dispatcher/store flows in place in part 2. Hence only requesting feedback for now, in case part 2 changes the shape of part 1 a bit.
Attachment #8495232 - Flags: feedback?(nperriault)
(Assignee)

Comment 10

3 years ago
Created attachment 8496734 [details] [diff] [review]
Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

Updated part 1, fixes a couple of extra issues, and updated to latest master.
Attachment #8495232 - Attachment is obsolete: true
Attachment #8495232 - Flags: feedback?(nperriault)
Attachment #8496734 - Flags: review?(nperriault)
Attachment #8496734 - Flags: review?(mdeboer)
(Assignee)

Comment 11

3 years ago
Created attachment 8496799 [details] [diff] [review]
Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client.

This part is all about setting up a basic dispatcher, actions and flow, to handle the initial pending call display, getting the data from the server, and then connecting to the websocket.

I've also included a change of state to terminated (e.g. for timeouts/rejections), to demonstrate the flows there.

Still hoping that we can pair review these later today.
Attachment #8496799 - Flags: review?(nperriault)
Attachment #8496799 - Flags: review?(mdeboer)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8496734 [details] [diff] [review]
Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

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

Notes from pair reviewing with Mike over Vidyo.

::: browser/components/loop/content/js/conversation.jsx
@@ +198,5 @@
>     * At the moment, it does more than that, these parts need refactoring out.
>     */
>    var IncomingConversationView = React.createClass({
>      propTypes: {
> +     client: React.PropTypes.instanceOf(loop.Client).isRequired,

indent typo

@@ +500,5 @@
> +                          .isRequired,
> +      sdk: React.PropTypes.object.isRequired,
> +
> +      // XXX New types for OutgoingConversationView
> +      conversationStore: React.PropTypes.instanceOf(loop.ConversationStore).isRequired

As this is the main store, lets go with calling the prop "store" for now, and keep it short.

@@ +509,5 @@
> +    },
> +
> +    render: function() {
> +      if (this.state.outgoing) {
> +        return (<loop.conversationViews.OutgoingConversationView

Consider making this a constant and dropping the namespace

::: browser/components/loop/content/js/conversationViews.jsx
@@ +61,5 @@
> +            <button className="btn btn-cancel">
> +              {mozL10n.get("initiate_call_cancel_button")}
> +            </button>
> +          <div className="fx-embedded-call-button-spacer"></div>
> +            </div>

indentation

::: browser/components/loop/content/shared/css/conversation.css
@@ +224,5 @@
>  }
>  
> +/* General Call (incoming or outgoing). */
> +
> +.call-action-group {

Move these back down to save blame

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +138,5 @@
> +      ccView = mountTestComponent();
> +
> +      TestUtils.findRenderedComponentWithType(ccView,
> +        loop.conversationViews.OutgoingConversationView);
> +

nit: unnecessary blank line

::: browser/components/loop/ui/index.html
@@ +26,5 @@
>        window.OTProperties.configURL = window.OTProperties.assetURL + 'js/dynamic_config.min.js';
>        window.OTProperties.cssURL = window.OTProperties.assetURL + 'css/ot.css';
>      </script>
>      <script src="../content/shared/libs/sdk.js"></script>
> +    <script src="../content/shared/libs/react-0.11.1-prod.js"></script>

Drop this change for now.

@@ +58,5 @@
>      </script>
>      <script src="../content/js/panel.js"></script>
>      <script src="../content/js/conversation.js"></script>
>      <script src="ui-showcase.js"></script>
> + </body>

nit: bad indent change
(Assignee)

Comment 13

3 years ago
Comment on attachment 8496799 [details] [diff] [review]
Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client.

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

Comments from pair review with Mike over vidyo.

::: browser/components/loop/content/js/client.js
@@ +225,5 @@
> +     * @param {Array} calleeIds an array of emails and phone numbers.
> +     * @param {String} callType the type of call.
> +     * @param {Function} cb Callback(err, result)
> +     */
> +    requestOutgoingCallData: function(calleeIds, callType, cb) {

nit: rename to setupOutgoingCall

@@ +240,5 @@
> +
> +          var postData = JSON.parse(responseText);
> +
> +          // This throws if the data is invalid, in which case only the failure
> +          // telemetry will be recorded.

This comment shouldn't be here.

@@ +241,5 @@
> +          var postData = JSON.parse(responseText);
> +
> +          // This throws if the data is invalid, in which case only the failure
> +          // telemetry will be recorded.
> +          var outgoingCallData = this._validate(postData,

Needs a try/catch in case data is invalid

::: browser/components/loop/content/js/conversation.jsx
@@ +512,5 @@
> +      this.props.conversationStore.on("change:outgoing", this._onChange, this);
> +    },
> +
> +    _onChange: function() {
> +      this.setState(this.props.conversationStore.attributes);

Move this to an inline function.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +100,5 @@
> +      this.props.conversationStore.on("change", this._onChange, this);
> +    },
> +
> +    _onChange: function() {
> +      this.setState(this.props.conversationStore.attributes);

Make inline.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +23,4 @@
>        calleeId: undefined,
> +      // The call type for the call.
> +      // XXX Don't hard-code, this comes from the data in bug 1072323
> +      callType: "audio-video",

Define constants for callTypes

@@ +122,5 @@
> +        callState: "gather"
> +      });
> +
> +      if (this.get("outgoing")) {
> +        this._getOutgoingCallData();

change to _setupOutgoingCall

@@ +176,5 @@
> +
> +      this._websocket.promiseConnect().then(
> +        function() {
> +          this.dispatcher.dispatch(new sharedActions.ConnectionProgress({
> +            state: "connecting"

Add note, this is the call state.

@@ +207,5 @@
> +        case "alerting":
> +          action = new sharedActions.ConnectionProgress({
> +            state: progressData.state
> +          });
> +      }

nit: add break and a default case (log it)

::: browser/components/loop/content/shared/js/dispatcher.js
@@ +29,5 @@
> +     * @param {Object} store The store object to register
> +     * @param {Array} eventTypes An array of action names
> +     */
> +    register: function(store, eventTypes) {
> +      eventTypes.forEach(function (type) {

nit: no space after function

::: browser/components/loop/test/desktop-local/client_test.js
@@ +275,5 @@
> +          { calleeId: calleeIds, callType: callType }
> +        );
> +      });
> +
> +      it("should call the callback if the request is successful", function() {

Need a test for invalid data - copy test for "should send an error if the data is not valid"

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +108,5 @@
> +    it("should update the rendered views when the state is changed.",
> +      function() {
> +        conversationStore.set({callState: "connecting"});
> +
> +        view = mountTestComponent();

Check pending conversation view is displayed for test clarity

::: browser/components/loop/test/shared/conversationStore_test.js
@@ +252,5 @@
> +
> +      it("should dispatch a connection failure action on failure", function(done) {
> +        rejectConnectPromise();
> +
> +        connectPromise.then(function() {}, function() {

Consider doing an assert in the success case, to be explicit about test failure.

::: browser/components/loop/test/shared/dispatcher_test.js
@@ +49,5 @@
> +      });
> +
> +      cancelAction = new sharedActions.CancelCall();
> +
> +      gather1 = {

Rename these to gatherStore1 etc

@@ +102,5 @@
> +      beforeEach(function() {
> +        // Restore the stub, so that we can easily add a function to be
> +        // returned. Unfortunately, sinon doesn't make this easy.
> +        cancel1.cancelCall.restore();
> +        sandbox.stub(cancel1, "cancelCall", function() {

Can we clean this up by replacing functions or some other means?

@@ +110,5 @@
> +          sinon.assert.notCalled(gather2.gatherCallData);
> +        });
> +      });
> +
> +      it("should not dispatch an action if the previous action hasn't finished", function() {

Add some notes to explain these tests better.

::: browser/components/loop/test/shared/validate_test.js
@@ +4,5 @@
> +/*global chai, validate */
> +
> +var expect = chai.expect;
> +
> +describe('Validator', function() {

nit: use double quotes
(Assignee)

Comment 14

3 years ago
Created attachment 8497471 [details] [diff] [review]
[checked in] Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

Updated to address review comments. The only extra thing I change was to reset the title of the ui-showcase, as I noticed some of the views were changing it. To make this work, I had to pass a dummy websocket instance to one of the views to avoid exceptions, hopefully that will go away soon.
Attachment #8496734 - Attachment is obsolete: true
Attachment #8496734 - Flags: review?(nperriault)
Attachment #8496734 - Flags: review?(mdeboer)
Attachment #8497471 - Flags: review?(mdeboer)
(Assignee)

Comment 15

3 years ago
Created attachment 8497581 [details] [diff] [review]
[checked in] Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client.

Updated per the review comments.
Attachment #8496799 - Attachment is obsolete: true
Attachment #8496799 - Flags: review?(nperriault)
Attachment #8496799 - Flags: review?(mdeboer)
Attachment #8497581 - Flags: review?(mdeboer)
Comment on attachment 8497471 [details] [diff] [review]
[checked in] Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

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

Just like we discussed :) Thanks!

::: browser/components/loop/content/js/conversation.jsx
@@ +12,5 @@
>    "use strict";
>  
>    var sharedViews = loop.shared.views,
> +      sharedModels = loop.shared.models,
> +      OutgoingConversationView = loop.conversationViews.OutgoingConversationView;

nit: Since you're touching this, could you put each statement on its own line with a `var`?
Attachment #8497471 - Flags: review?(mdeboer) → review+
Comment on attachment 8497581 [details] [diff] [review]
[checked in] Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client.

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

\o/ This is going to be grand.

::: browser/components/loop/content/js/client.js
@@ +237,5 @@
> +            this._failureHandler(cb, err);
> +            return;
> +          }
> +
> +          var postData = JSON.parse(responseText);

JSON.parse can throw... I don't think hawkRequest() catches that, right?
Attachment #8497581 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 18

3 years ago
Pushed the first two parts with the final comments fixed:

https://hg.mozilla.org/integration/fx-team/rev/326b3f37e469
https://hg.mozilla.org/integration/fx-team/rev/5d2c69ebb321
Keywords: leave-open
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
https://hg.mozilla.org/mozilla-central/rev/326b3f37e469
https://hg.mozilla.org/mozilla-central/rev/5d2c69ebb321
(Assignee)

Comment 20

3 years ago
Created attachment 8498149 [details] [diff] [review]
Part 3 - Finish the view flow transition for direct calling for Loop.

I was going to hook up the sdk parts of this patch, but it seemed simpler just to get the entire view flow in place and then add in the sdk parts in the fourth part.

Hence this adds in flows for:

- transitioning to a vastly non-functional ongoing call view
- implementing hangup call going to the feedback view
- implementing retry and cancel buttons on the call failed view
- implementing cancel on the pending call view

The styles are wrong, I'm proposing we fix that later once we've got everything working (or alongside the next patch maybe).
Attachment #8498149 - Flags: review?(nperriault)
(Assignee)

Comment 21

3 years ago
Created attachment 8498777 [details] [diff] [review]
[checked in] Part 3 - Finish the view flow transition for direct calling for Loop.

Updated patch based on direct pair review comments over vidyo.
Attachment #8498149 - Attachment is obsolete: true
Attachment #8498149 - Flags: review?(nperriault)
Attachment #8498777 - Flags: review?(nperriault)
(Assignee)

Updated

3 years ago
Attachment #8497471 - Attachment description: Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view. → [checked in] Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.
(Assignee)

Updated

3 years ago
Attachment #8497581 - Attachment description: Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client. → [checked in] Part 2 - Set up actions and a dispatcher and start to handle obtaining call data for outgoing Loop calls from the desktop client.
Comment on attachment 8498777 [details] [diff] [review]
[checked in] Part 3 - Finish the view flow transition for direct calling for Loop.

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

Pair reviewed over Vidyo, some supplementary nits you may want to address before landing. Great work!

::: browser/components/loop/content/js/conversationViews.jsx
@@ +48,3 @@
>        callState: React.PropTypes.string,
>        calleeId: React.PropTypes.string,
> +      enableCancelButton: React.PropTypes.bool.isRequired

Nit: I'd rather see it being optional and have a default set through getDefaultProps()

@@ +281,5 @@
> +            dispatcher={this.props.dispatcher}
> +            callState={this.state.callState}
> +            calleeId={this.state.calleeId}
> +            enableCancelButton={this.state.callState !== CALL_STATES.INIT &&
> +                                this.state.callState != CALL_STATES.GATHER}

Why mixing !== and != here?

Also please move this test to a private method, eg. isCancellable() or equivalent.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +27,5 @@
> +    // One of the two parties has indicated successful media set up,
> +    // but the other has not yet.
> +    HALF_CONNECTED: "half-connected",
> +    // Both endpoints have reported successfully establishing media.
> +    CONNECTED: "connected"

While we're now prefixing call states with "cs-", how about prefixing these with "ws-" for consistency and killing any possible confusion?

@@ +241,5 @@
> +     * Retries a call
> +     */
> +    retryCall: function() {
> +      var callState = this.get("callState");
> +      if (!callState === CALL_STATES.TERMINATED) {

if (callState !== CALL_STATES.TERMINATED) is probably what we want :)

::: browser/components/loop/ui/ui-showcase.jsx
@@ +265,5 @@
>                  <StartConversationView conversation={mockConversationModel}
>                                         client={mockClient}
>                                         notifications={notifications} />
>                </div>
>              </Example>

Please note the UI showcase shows an error in the console as loop.Dispatcher is undefined; you need to fake it or to include the script into that page.
Attachment #8498777 - Flags: review?(nperriault) → review+
(Assignee)

Comment 23

3 years ago
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #22)
> > +    CONNECTED: "connected"
> 
> While we're now prefixing call states with "cs-", how about prefixing these
> with "ws-" for consistency and killing any possible confusion?

I didn't do this as currently these map directly to the websocket states, as received from the websocket. To change these would mean we'd need to be tweaking them in various places as they are received.

> Please note the UI showcase shows an error in the console as loop.Dispatcher
> is undefined; you need to fake it or to include the script into that page.

Fixed by making the showcase not run the init functions for the panel and conversation files.
(Assignee)

Comment 24

3 years ago
Comment on attachment 8498777 [details] [diff] [review]
[checked in] Part 3 - Finish the view flow transition for direct calling for Loop.

https://hg.mozilla.org/integration/fx-team/rev/248b7b6c41d5
Attachment #8498777 - Attachment description: Part 3 - Finish the view flow transition for direct calling for Loop. → [checked in] Part 3 - Finish the view flow transition for direct calling for Loop.
https://hg.mozilla.org/mozilla-central/rev/248b7b6c41d5
(Assignee)

Comment 26

3 years ago
Created attachment 8499614 [details] [diff] [review]
Part 4 - Hook up the OT sdk to the direct calling window for Loop.

This completes everything necessary to hook up the sdk and have the call completely working.

There's still issues like styles and other things, but I'll file follow-up bugs for those.
Attachment #8499614 - Flags: review?(nperriault)
(Assignee)

Updated

3 years ago
Blocks: 1000178
(Assignee)

Updated

3 years ago
Blocks: 1000183
(Assignee)

Updated

3 years ago
Blocks: 1015855
(Assignee)

Updated

3 years ago
Blocks: 1048161
(Assignee)

Updated

3 years ago
No longer depends on: 1066568
Comment on attachment 8499614 [details] [diff] [review]
Part 4 - Hook up the OT sdk to the direct calling window for Loop.

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

This looks great. r=me conditional to comments being addressed before landing, nits are optional.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +143,5 @@
>      },
>  
> +    // height set to 100%" to fix video layout on Google Chrome
> +    // @see https://bugzilla.mozilla.org/show_bug.cgi?id=1020445
> +    publisherConfig: {

Nit: Please create a getPublisherConfig method, see below.

@@ +162,5 @@
>        };
>      },
>  
> +    componentWillMount: function() {
> +      this.publisherConfig.publishVideo = this.props.video.enabled;

Nit: This could be avoided by creating a getPublisherConfig() method, keep reading.

@@ +179,5 @@
> +      // The SDK needs to know about the configuration and the elements to use
> +      // for display. So the best way seems to pass the information here - ideally
> +      // the sdk wouldn't need to know this, but we can't change that.
> +      this.props.dispatcher.dispatch(new sharedActions.SetupStreamElements({
> +        publisherConfig: this.publisherConfig,

Nit: Here we could call the getPublisherConfig() method mentioned previously, which could easily return a config object with a `publishVideo` property matching the current `video.enabled` prop value.

@@ +207,2 @@
>      updateVideoContainer: function() {
>        var localStreamParent = document.querySelector('.local .OT_publisher');

document? that should be this.getDOMNode().querySelector(".local")

BTW that means we could probably reuse the _getElement method here.

::: browser/components/loop/content/shared/js/actions.js
@@ +111,5 @@
> +     */
> +    MediaConnected: Action.define("mediaConnected", {
> +    }),
> +
> +    SetMute: Action.define("setMute", {

Missing jsdocs.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +176,3 @@
>          case WS_STATES.HALF_CONNECTED:
>          case WS_STATES.CONNECTED: {
> +          // Nothing to do.

Nit: Ensuring that callState is set to CALL_STATES.ONGOING is probably safe here, and shouldn't trigger a component rerender if shouldComponentUpdate is properly implemented.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +206,5 @@
> +    /**
> +     * Publishes the local stream if the session is connected
> +     * and the publisher is ready.
> +     */
> +    _maybePublishLocalStream: function() {

Nit: This could be a monad, we're ready for haskell :p

@@ +213,5 @@
> +        this.session.publish(this.publisher);
> +
> +        // Now record the fact, and check if we've got all media yet.
> +        this._publishedLocalStream = true;
> +        this._checkAllStreamsConnected();

Nit: I'd rather dispatch the action from here, but would keep the test in a meaningfully named method:

if (this._checkAllStreamsConnected()) {
  this.dispatcher.dispatch(new sharedActions.MediaConnected());
}

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +187,5 @@
> +        var hangupBtn = view.getDOMNode().querySelector('.btn-hangup');
> +
> +        React.addons.TestUtils.Simulate.click(hangupBtn);
> +
> +        sinon.assert.called(dispatcher.dispatch);

Nit: Any specific reason we're not using calledOnce here? If so, sinon.assert.calledWithMatch implicitely test for called() already, so this could be removed.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +15,5 @@
> +  beforeEach(function() {
> +    sandbox = sinon.sandbox.create();
> +
> +    fakeElement1 = {fake: 1};
> +    fakeElement2 = {fake: 2};

For clarity, these should be renamed fakeLocalElement and fakeRemoteElement respectively.
Attachment #8499614 - Flags: review?(nperriault) → review+
(Assignee)

Comment 28

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/b28e496d535b
Keywords: leave-open
(Assignee)

Comment 29

3 years ago
Although this has landed, it isn't accessible until bug 1072323 is fixed.

However, there is currently a way of testing this without bug 1072323. It would be good to start getting some advance testing on the flow. 

1) Start up two nightlies, sign into loop via FxA on each, with different accounts.
2) On one of the nightlies 'A', set the pref "loop.outgoingemail" to the account email of the other nightly.
3) On nightly 'A' with the pref set, copy a call url from the panel.
4) Open that in nightly 'B' browser, and start the call

=> At this stage an outgoing call will appear on nightly 'A', and start a call, so that 'nightly' B receives an incoming call.

Cancelling the call url (standalone UI) won't do anything. In this state, you've effectively initiated the outgoing call.

With outgoing calls, you can:

- Accept the call
- Connect the call and have a conversation
- Give feedback at both ends when the conversation is finished
- Be notified if the call is rejected/fails for some reason (with a generic failure message)
- Retry the call if it fails

Most of these are also covered in bugs that this block, that I'll be marking as fixed.
Flags: needinfo?(anthony.s.hughes)
(Assignee)

Updated

3 years ago
Blocks: 1077710
My current priority is making sure all the ducks are lined up for Firefox 34 moving to Beta in roughly a week. Paul, can you do some testing of this in the latest Nightly builds based on comment 29?
Flags: needinfo?(anthony.s.hughes) → needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes
https://hg.mozilla.org/mozilla-central/rev/b28e496d535b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Iteration: --- → 35.3
Flags: qe-verify?
(Assignee)

Comment 32

3 years ago
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #30)
> My current priority is making sure all the ducks are lined up for Firefox 34
> moving to Beta in roughly a week.

FYI We're expecting this to be uplifted before the merges.

> Paul, can you do some testing of this in
> the latest Nightly builds based on comment 29?

Note that a test case I forgot would be to log in on Firefox Nightly, and then direct call to the Firefox OS application. I tested this on Friday before around the time that this landed.
(In reply to Mark Banner (:standard8) from comment #29)
> However, there is currently a way of testing this without bug 1072323. It
> would be good to start getting some advance testing on the flow.
Possible issues:
1. 'retry/cancel' buttons are smaller than 'cancel/accept' buttons from the incoming call window
2. bug 1078261 - not sure if related
Flags: needinfo?(paul.silaghi)
(Assignee)

Comment 34

3 years ago
(In reply to Paul Silaghi, QA [:pauly] from comment #33)
> (In reply to Mark Banner (:standard8) from comment #29)
> > However, there is currently a way of testing this without bug 1072323. It
> > would be good to start getting some advance testing on the flow.
> Possible issues:
> 1. 'retry/cancel' buttons are smaller than 'cancel/accept' buttons from the
> incoming call window

Bug 1077710

> 2. bug 1078261 - not sure if related

I suspect that's a separate issue (I just commented there).
Thanks Mark.
Marking as verified fixed FF 35.0a1 (2014-10-05).
Status: RESOLVED → VERIFIED
status-firefox35: --- → verified
Comment on attachment 8497471 [details] [diff] [review]
[checked in] Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8497471 - Flags: approval-mozilla-aurora?

Updated

3 years ago
Attachment #8497581 - Flags: approval-mozilla-aurora?

Updated

3 years ago
Attachment #8498777 - Flags: approval-mozilla-aurora?

Updated

3 years ago
Attachment #8499614 - Flags: approval-mozilla-aurora?
Flags: qe-verify? → qe-verify+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a29ef7c8a3e0
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc8fca136da3
https://hg.mozilla.org/releases/mozilla-aurora/rev/4fda0b154861
https://hg.mozilla.org/releases/mozilla-aurora/rev/669ecc39ceae

Updated

3 years ago
status-firefox34: --- → fixed
Flags: needinfo?(paul.silaghi)
Comment on attachment 8497471 [details] [diff] [review]
[checked in] Part 1 - Add a new controller view for selecting between incoming and outgoing calls in the Loop Conversation window. Also, set up a bare-bones outgoing pending conversation view.

Approval previously granted to jesup to land this change as part of the second batch of Loop fixes to land on Aurora. Marking the approval after the fact.
Attachment #8497471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8497581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8498777 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8499614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FF 34b1 Win 7
status-firefox34: fixed → verified
Flags: needinfo?(paul.silaghi)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.