Closed Bug 985596 Opened 6 years ago Closed 6 years ago

Set up initial desktop conversation window

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33
Blocking Flags:
backlog mlp+

People

(Reporter: standard8, Assigned: standard8)

References

Details

User Story

When a person clicks a link that I send them, I should be able to receive the call and have a conversation with them.

Approximate UX:

- Conversation window opens (bug 981064)
- Prompted to accept or reject call
- Permission Requested for media
- On accepting the permission, local video is displayed, along with remote video once connection is established.

Exclusions:

- No audio-only, no face mute, no remote audio mute

Attachments

(6 files, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
We need to set up the initial conversation window in the client.

The attached patch sets up a chat.html which is loaded into the chat window.

On initialisation this also goes and talks to the server to try and get the /calls/ information. However that doesn't work - it seems like xhrs don't like posting bodies with GETs which is what the server expects (I've tried xmlhttprequest as per the version in the patch currently, as well as jQuery versions).

To test this, I've been running the servers getting the call url, extracting the token from the call url and then doing a curl request:

curl -v -i  --cookie "loop-session={cookie}" --header "Content-Type:application/x-www-form-urlencoded" -X POST http://localhost:5000/calls/{token}

The cookie I've also got from examining the call-url request in the browser console and copy pasting.
Priority: -- → P1
Assignee: nobody → standard8
User Story: (updated)
Duplicate of this bug: 979890
This sets up the basic conversation window code infrastructure.

It also initialises the conversation window and loads the call information from the server, read for processing.

There's a couple of XXX comments:

- One for the "version" field sent to the server in the request. We probably want to make the last time checked either a pref or local storage.

- One for the fact the calls request is in the conversation window. We probably want it to end up in the background service, and somehow passed to the conversation window when it opens.

In both these cases, I think its going to be easier to make a decision on how they happen once we know what our chrome versus content picture looks like, and what we have will be sufficient for a demo.

If we're happy with that, then I'll raise bugs for those before this lands.

The next steps will be to integrate with code that's happening on the link clicker side to start hooking up the call info to the media streams.
Attachment #8393651 - Attachment is obsolete: true
Attachment #8394908 - Flags: review?(adam)
FWIW I've been testing this in the same manner as described in comment 0
The HEAD of loop-server now requires that a nickname be posted as a parameter.  This means the curl statement used to test this against the latest server bits now needs to look something like this:

curl -v -i  --cookie "loop-session={cookie}" --header "Content-Type:application/x-www-form-urlencoded" -X POST http://localhost:5000/calls/{token} --data 'nickname=johndoe'

The code is going to need some tweaking too.
https://gist.github.com/anonymous/9750544 is a dirty hack I did to get the conversation window opening again.
Although, in theory, I'm not sure that actually _ought_ to work, since it's putting the param in the header rather than the body of the POST.
I see why the broken gist "works".  requestCallUrl (singular) is never called except by test code.  Please file a separate bug to update the tests and that function would be good to do as part of the landing process for this code.
Comment on attachment 8394908 [details] [review]
[checked in] Basic conversation window and calls request

r=dmose, with the tweaks suggested in the github PR reviewed and the commits squashed.
Attachment #8394908 - Flags: review?(adam) → review+
BTW, seeing things get this far is exciting; nice work!
dmose we just land https://bugzilla.mozilla.org/show_bug.cgi?id=986533 so you don't need your hack anymore.
(In reply to Dan Mosedale (:dmose) from comment #7)
> I see why the broken gist "works".  requestCallUrl (singular) is never
> called except by test code.  Please file a separate bug to update the tests
> and that function would be good to do as part of the landing process for
> this code.

requestCallUrl is called from panel.js, so we don't need a separate bug for this, although it looks like we may need a follow-up bug from bug 986533 landing.
Comment on attachment 8394908 [details] [review]
[checked in] Basic conversation window and calls request

https://github.com/adamroach/gecko-dev/commit/6ce0fb6b2461f495d24e70edaac7dcb859ec5c78
Attachment #8394908 - Attachment description: Basic conversation window and calls request → [checked in] Basic conversation window and calls request
I've now pushed some WIP branches. Both have YYY comments in them, for things that need be done to get them ready for review.

First loop-client:

https://github.com/mozilla/loop-client/compare/desktop_additions

This adds three files:

- Modified SDK to cope with being loaded in a chrome space. This is temporarily in shared, as loop-client is private. Hopefully once we get the chrome vs content set-up, we can unmodify this, remove it or whatever.

- views.js: iirc these are copy-pasted versions from webapp.js. So webapp.js should be made to use these views & the tests updated/moved to match.

- models.js: This is a modified version of the model. Some merging will need to be done to get webapp.js and the desktop client working together on this one. Some of the router work (see below) in the desktop client may help this.

Next, gecko-dev:

https://github.com/adamroach/gecko-dev/compare/loop-ui-initial...establish-call

The changes here are mainly hooking everything in from the shared directories. Notes:

- conversation.js may want to be rewritten as a backbone Router, or some other appropriate item. See YYY comments in file - Niko probably has the best ideas here.

- Tests won't work as they are running in content space rather than chrome. Having just thought about it, we could probably stub the chrome stuff to get them to work. Again, once we get chrome vs content info, this may change.

- There's probably also some tests need writing/updating; especially for conversation.js (LoopService is something I don't think we should land in master, just a glue for now).
This moves some of the models that the webapp uses to the shared/ directory so that the desktop client can use them.

The models are unchanged after the move. There may be some optimisations to come later, but we'll improve those in the next cycles.

The establish-call branch in gecko-dev has also been updated to work with the changes here.
Attachment #8398561 - Flags: review?(dmose)
To do for establish-call (https://github.com/adamroach/gecko-dev/compare/loop-ui-initial...establish-call):

There's still some YYY's left, namely unit tests for conversation.js and Improving loop.conversation.Router.start.

Once we've done those, I think it would be reasonable to land it in the current form.
Comment on attachment 8398561 [details] [review]
[checked in] Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6

r+, conditional on Mark looking over and being ok with or adjusting the code I wrote to address my review comments.
Attachment #8398561 - Flags: review?(dmose) → review+
Re the establish-call branch changes: in the future, it would make it easier to see what changes you've made since the previous iteration if you avoided squashing commits until there's a fairly compelling reason to do it (e.g. landing the patch).
It's not totally obvious how to approach the unit test bits of conversation.js itself, given that's not yet shared.  I'm assuming it will be, but I have a vague recollection of a convo with Mark earlier this week suggesting it might not. 

If it will be, does it make sense to start with whichever version of conversation.js we expect to be shared, write unit tests for how we expect the shared version to look, making them pass as we go?  I.e. effectively TDDing the merge.
Niko and Mark, I'd love to have a chat about unit testing the views on Monday AM pacific time, if that's possible.  I kinda feel like DOM tests with fixtures (like Niko did towards the end of Talkilla, and despite the chunks of duplicated HTML in the tests) may be the least of the available evils here, but I'd like to see if we can't come to some sort of agreement here...
Agreed having DOM integration tests would be nice. I'll set up some in the next few days.

I'm more concerned with the shared assets tests right now, and they'll be run on m-c. I'm about to add a new symlink for shared/test into the gecko tree for the time being, so shared tests can at least be executed from gecko.
(In reply to Dan Mosedale (:dmose) from comment #16)
> Comment on attachment 8398561 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6
> 
> r+, conditional on Mark looking over and being ok with or adjusting the code
> I wrote to address my review comments.

The additional changes were fine.
Comment on attachment 8398561 [details] [review]
[checked in] Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6

https://github.com/mozilla/loop-client/commit/381b86fbf6b6f749493bdf2011f6a9e565944c34
https://github.com/mozilla/loop-client/commit/68abc2b7cfbfc9563fbc331926611ffe7f9fb3d8
Attachment #8398561 - Attachment description: Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6 → [checked in] Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6
Updated shared assets layout to ease sharing with desktop client.
Attachment #8399452 - Flags: review?(standard8)
Note that to use pull 7 linked to the in above comment, you'll probably want to update your gecko-dev to this branch: https://github.com/adamroach/gecko-dev/tree/loop-shared-content-refactor/browser/components/loop

After wasting time fighting with symlinks, I've pushed a simple script (make-links.sh) to that branch to make the setup less error-prone, and updated the README.
This PR is based on loop-client PR #7, which should land first.

It moves client.js to be a shared file (source is currently the desktop files, updating those to be handled in the establish-call branch).

It then extends client.js to include the request that ConversationModel.initiate is currently using, and then makes ConversationModel use that new request, or a different one, depending on if it is an incoming or outgoing call.

I suggest reviewing all of client.js and client_tests.js as there has been rework and tidy up over all of the files.
Attachment #8399632 - Flags: review?(dmose)
Comment on attachment 8399452 [details] [review]
[checked in] https://github.com/mozilla/loop-client/pull/7

Looks good!  r=dmose, conditional on a few things mentioned in the PR.

Please talk to Standard8 about how to land this best. We want to cause the least disruption to folks who are currently using the loop-ui-initial branch to test (if that's anyone).  It's possible that that is actually noone, since the test call only worked on the desktop_additions branch before.

In any case, I expect it'll at least make sense to send out an email to webrtc-internal telling folks which branches to use, and to run the make-links.sh script to update their links.
Attachment #8399452 - Flags: review?(standard8) → review+
This patch reflects the changes made in loop-client PR 7 to the Desktop client.
Attachment #8400046 - Flags: review?(standard8)
Comment on attachment 8400046 [details] [review]
[checked in] https://github.com/adamroach/gecko-dev/pull/9

Looks good, r=Standard8. Please squash commits & add bug number when merging.
Attachment #8400046 - Flags: review?(standard8) → review+
Niko, I got confused today because loop-shared-content-refactor was still around (so I didn't realized that something equivalent had already been merged to loop-ui-initial).  Would you garbage-collect the various loop-related branches sitting around on adamroach/gecko-dev that are no longer needed?
Comment on attachment 8399632 [details] [review]
[checked in] Handle incoming and outgoing calls in the conversation model

r=dmose with the details in the PR addressed/responded to
Attachment #8399632 - Flags: review?(dmose) → review+
Attachment #8400046 - Attachment description: https://github.com/adamroach/gecko-dev/pull/9 → [checked in] https://github.com/adamroach/gecko-dev/pull/9
Attachment #8399452 - Attachment description: https://github.com/mozilla/loop-client/pull/7 → [checked in] https://github.com/mozilla/loop-client/pull/7
Comment on attachment 8399632 [details] [review]
[checked in] Handle incoming and outgoing calls in the conversation model

https://github.com/mozilla/loop-client/commit/c68d7e3060f8244fe16d37101d6004c0160854ee
https://github.com/mozilla/loop-client/commit/ba30072c78f11134d6841aabdad9b77bbce26ec4
Attachment #8399632 - Attachment description: Handle incoming and outgoing calls in the conversation model → [checked in] Handle incoming and outgoing calls in the conversation model
This gets a basic conversation window up and running - when a push notification is received, the conversation window will be opened and automatically start the call (controls to come later). This makes it possible for the complete round-trip call to be set up.

This uses a lot of code from the shared directories, so the main glue here is the html and the ConversationRouter.

Client.js has moved out to the shared directory and is in loop-client already.

I'm not intending that we land LoopService.js without major rework, but I cleaned it up a bit anyway so that it didn't have unnecessary comments & debug.
Attachment #8400187 - Flags: review?(dmose)
(In reply to Mark Banner (:standard8) from comment #35)
> I'm not intending that we land LoopService.js without major rework, but I
> cleaned it up a bit anyway so that it didn't have unnecessary comments &
> debug.

"land" as in "land on master".

One thing I intend to do tomorrow is to go through the code, and ensure we've got appropriate bugs filed for XXX and things to follow-up on (sorting out LoopService is one of those).
Comment on attachment 8400187 [details] [review]
[checked in] Get the basic call working

r=dmose, once the various comments in the PR are either responded to or addressed.  Note that when this is merged, it'll be worth sending out mail to developers telling them that they'll need to make a change in their development.json equivalent to <https://github.com/mozilla-services/loop-server/pull/62/files>.
Attachment #8400187 - Flags: review?(dmose) → review+
Comment on attachment 8400187 [details] [review]
[checked in] Get the basic call working

https://github.com/adamroach/gecko-dev/commit/fd16e4fe4ede324a205b5bfa35738695da4982be
https://github.com/adamroach/gecko-dev/commit/2be32f330e6eeea9643ef8f1f66bdfadc03bb833
Attachment #8400187 - Attachment description: Get the basic call working → [checked in] Get the basic call working
Whiteboard: [landed on loop-ui-initial]
Duplicate of this bug: 990714
Depends on: 976109
backlog: --- → mlp+
Marking fixed as this has landed on loop-ui-initial and we believe the work here is done; loop-ui-initial will be cherry-picked to master at an appropriate time.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [landed on loop-ui-initial]
Group: mozilla-employee-confidential
Target Milestone: --- → mozilla33
Marking verified fixed from previous smoketesting.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
You need to log in before you can comment on or make changes to this bug.