Closed Bug 1072323 Opened 10 years ago Closed 10 years ago

Implement the Video/Audio Call buttons for contacts, to open up a conversation window

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
5

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [loop-uplift])

Attachments

(1 file, 2 obsolete files)

For the direct calling from desktop to other clients, we need to be able to initiate a call to a contact.

We need:

- Audio/Video call buttons on the sub-menu for the contact
https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts

- The buttons should make a call via the mozLoop api to the MozLoopService

- The service should then open the conversation window and pass to it a call id

- The conversation window should then use the call id to obtain the conversation details. This is an existing mechanism that's used for incoming calls (see _processCall)

I think the conversation details should contain something like:

{
  type: "outgoing",
  calleeId: <email>,
  calleeName: <contact name>
}

- We might need to do some handling for the "busy" status, or we could push that out to a different bug.
Blocks: 972015
Points: --- → 5
Flags: firefox-backlog+
Flags: qe-verify?
Whiteboard: [loop-uplift]
Flags: qe-verify? → qe-verify+
Mike, I've kinda throw this together, and it depends on other patches. What I'm looking for feedback on is the general idea, and if I'm doing things in the right places or not.

I suspect I possibly should be getting the display name at the contacts.jsx end of the flow, and maybe extracting the email addresses there as well - what do you think, do we have existing functions?
Attachment #8499167 - Flags: feedback?(mdeboer)
Comment on attachment 8499167 [details] [diff] [review]
Experimental patch for hooking up contacts to outgoing calls

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +572,5 @@
> +     * @param {String} callType The type of call, e.g. "audio-video" or "audio-only"
> +     * @return true if the call is opened, false if it is not opened (i.e. busy)
> +     */
> +    startDirectCall: {
> +      enumerate: true,

You might want to rename this to 'enumerable'

@@ +575,5 @@
> +    startDirectCall: {
> +      enumerate: true,
> +      writable: true,
> +      value: function(contactNames, contactAddresses, callType) {
> +        MozLoopService.startDirectCall(contactNames, contactAddresses, callType);

From an API perspective, this doesn't really make sense - two separate arrays. I'd expect a simple object like `{'name1': 'address1', 'name2': 'address2', ...}`

::: browser/components/loop/content/js/contacts.jsx
@@ +63,5 @@
>        return (
>          <ul className={cx({ "dropdown-menu": true,
>                              "dropdown-menu-up": this.state.openDirUp })}>
>            <li className={cx({ "dropdown-menu-item": true,
> +                              "disabled": false })}

You can simply remove the prop... it's not configurable with a prop (yet).

@@ +304,5 @@
>        this.props.startForm("contacts_add");
>      },
>  
> +    callContact: function(contact, callType) {
> +      navigator.mozLoop.startDirectCall(contact.name, contact.email, callType);

Alright, I see why you pass this data separately. So let's re-iterate... why not send and array of contacts if there is an option for multiple contacts?

If there's only one contact to ever start a direct call with, why not send the contact object as the first argument? The data structure is defined and won't change, so I think it's safe to pass around.

@@ +323,5 @@
>              }
>            });
>            break;
> +        case "video-call":
> +          this.callContact(contact, CALL_TYPES.AUDIO_VIDEO);

... and in that case you can simply do
`navigator.mozLoop.startDirectCall(contact, CALL_TYPES.AUDIO_VIDEO);`

::: browser/components/loop/content/js/conversationViews.jsx
@@ +23,5 @@
>     */
>    var ConversationDetailView = React.createClass({
>      propTypes: {
> +      contactNames: React.PropTypes.array,
> +      contactAddresses: React.PropTypes.array,

An array of contact objects would make this so much nicer AND future-proof
Attachment #8499167 - Flags: feedback?(mdeboer) → feedback+
Assignee: nobody → standard8
Iteration: --- → 35.3
Target Milestone: --- → mozilla35
This updates the patch according to the comments. Passing the contact around is definitely much nicer, and more logical.

Note there's also a couple of tweaks to the mute/enabled values - as I've now got easy access to try out audio-only calls, I found a few issues with them and hence the tweaks fix the logic.
Attachment #8499167 - Attachment is obsolete: true
Attachment #8500431 - Flags: review?(mdeboer)
Comment on attachment 8500431 [details] [diff] [review]
Hook up the contact menus to be able to start outgoing calls.

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

Just some stuff I found during a dry review pass:

::: browser/components/loop/content/js/conversationViews.jsx
@@ +65,5 @@
>    var PendingConversationView = React.createClass({
>      propTypes: {
>        dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired,
>        callState: React.PropTypes.string,
> +      contact: React.PropTypes.array,

Isn't this an object?

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +61,5 @@
> +        view = mountTestComponent({contact: contact});
> +
> +        expect(TestUtils.findRenderedDOMComponentWithTag(
> +          view, "h2").props.children).eql("fakeEmail");
> +      });

nit: this indentation made count all the braces... perhaps it's good to put the closing bracket on a newline?

::: browser/components/loop/test/xpcshell/test_loopservice_directcall.js
@@ +49,5 @@
> +  MozLoopService.releaseCallData(callId);
> +});
> +
> +function run_test()
> +{

nit: one-true-brace style is not something we'd like to move towards ;)

::: browser/components/loop/test/xpcshell/xpcshell.ini
@@ +17,5 @@
>  [test_loopservice_token_invalid.js]
>  [test_loopservice_token_save.js]
>  [test_loopservice_token_send.js]
>  [test_loopservice_token_validation.js]
> +[test_loopservice_busy.js]

why does splinter think this is a change?
Comment on attachment 8500431 [details] [diff] [review]
Hook up the contact menus to be able to start outgoing calls.

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

This is so awesome... I just called myself!

The only new thing I saw was the following error/ warning in the log:

"getLoopCharPref had trouble getting ot.guid; exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///modules/loop/MozLoopService.jsm :: this.MozLoopService.getLoopCharPref :: line 1385"  data: no]"

You might want to check that out. It could very well be an unrelated problem with the conversation window.
Attachment #8500431 - Flags: review?(mdeboer) → review+
Fixed review comments, carrying over r=mikedeboer.
Attachment #8500431 - Attachment is obsolete: true
Attachment #8501165 - Flags: review+
Attachment #8501165 - Attachment description: Ptch v2: Hook up the contact menus to be able to start outgoing calls. → Patch v2: Hook up the contact menus to be able to start outgoing calls.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9a5c24c5e36a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> The only new thing I saw was the following error/ warning in the log:
> 
> "getLoopCharPref had trouble getting ot.guid; exception: [Exception...
> "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
> [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" 
> location: "JS frame :: resource:///modules/loop/MozLoopService.jsm ::
> this.MozLoopService.getLoopCharPref :: line 1385"  data: no]"
> 
> You might want to check that out. It could very well be an unrelated problem
> with the conversation window.

That should just be a first-time issue. "ot.guid" is something we save on behalf of the sdk.
Paul: This lets you test the outgoing conversation window from the contacts list. It should be in today's nightly when it is generated.
Flags: needinfo?(paul.silaghi)
I think one problem would be if I'm signed-in with the same account on multiple profiles (devices) and get called - the incoming call windows shows up on everyone of them. Canceling the call on one device closes the main call on the caller's side.
Flags: needinfo?(paul.silaghi)
Other issues:
1. I'm allowed to call a person who doesn't have me in its contacts list
2. I'm allowed to call a person who have have me in its contacts list as 'blocked'
3. while the outgoing call window is open in the 'retry/cancel/ state, edit the email of the person you've just called and click 'retry' -> the call is successfully made and it shouldn't
Flags: needinfo?(standard8)
This broke the ui-showcase, I pushed a fix with rs=Niko over irc to fix it (npotb):

https://hg.mozilla.org/integration/fx-team/rev/d5ce88ef74a5
(In reply to Paul Silaghi, QA [:pauly] from comment #13)
> Other issues:
> 1. I'm allowed to call a person who doesn't have me in its contacts list

Yes, that's intentional. See bug 1000142 for a future privacy limitation.

> 2. I'm allowed to call a person who have have me in its contacts list as
> 'blocked'

This seems not to be implemented yet (looks like we haven't hooked up everything), so please file it.

> 3. while the outgoing call window is open in the 'retry/cancel/ state, edit
> the email of the person you've just called and click 'retry' -> the call is
> successfully made and it shouldn't

That's an edge case I think, I'm not sure what we should do, so please file a bug for now.
Flags: needinfo?(standard8)
2 - bug 1079941
3 - bug 1079964

Mark, when you can, please also take a look at comment 12.
Flags: needinfo?(standard8)
(In reply to Paul Silaghi, QA [:pauly] from comment #12)
> I think one problem would be if I'm signed-in with the same account on
> multiple profiles (devices) and get called - the incoming call windows shows
> up on everyone of them. Canceling the call on one device closes the main
> call on the caller's side.

Ah, missed this. I'm sure there's a bug on a related topic already, but I can't see it at a glance. I'm pretty sure that bug doesn't cover this case, so lets file it as well.
Flags: needinfo?(standard8)
Calling this verified fixed based on follow-up comments to Paul's testing.
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes → paul.silaghi
Comment on attachment 8501165 [details] [diff] [review]
Patch v2: Hook up the contact menus to be able to start outgoing calls.

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8501165 - Flags: approval-mozilla-aurora?
Flags: needinfo?(paul.silaghi)
Verified fixed FF 34b1 Win 7
Flags: needinfo?(paul.silaghi)
Comment on attachment 8501165 [details] [diff] [review]
Patch v2: Hook up the contact menus to be able to start outgoing calls.

Already landed in aurora, approving for posterity
Attachment #8501165 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.