985596 - Set up initial desktop conversation window

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: WIP

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.

Comment 2

[Mark Banner (:standard8)]

Attachment: [checked in] Basic conversation window and calls request

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.

Comment 3

[Mark Banner (:standard8)]

FWIW I've been testing this in the same manner as described in comment 0

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

https://gist.github.com/anonymous/9750544 is a dirty hack I did to get the conversation window opening again.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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 8

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

BTW, seeing things get this far is exciting; nice work!

Comment 10

[Rémy Hubscher (:natim)]

dmose we just land https://bugzilla.mozilla.org/show_bug.cgi?id=986533 so you don't need your hack anymore.

Comment 11

[Mark Banner (:standard8)]

(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 12

[Mark Banner (:standard8)]

Comment on attachment 8394908 [details] [review]
[checked in] Basic conversation window and calls request

https://github.com/adamroach/gecko-dev/commit/6ce0fb6b2461f495d24e70edaac7dcb859ec5c78

Comment 13

[Mark Banner (:standard8)]

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).

Comment 14

[Mark Banner (:standard8)]

Attachment: [checked in] Link to Github pull-request: https://github.com/mozilla/loop-client/pull/6

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.

Comment 15

[Mark Banner (:standard8)]

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 16

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

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).

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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.

Comment 21

[Mark Banner (:standard8)]

(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 22

[Mark Banner (:standard8)]

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

Comment 23

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: [checked in] https://github.com/mozilla/loop-client/pull/7

Updated shared assets layout to ease sharing with desktop client.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 25

[Mark Banner (:standard8)]

Attachment: [checked in] Handle incoming and outgoing calls in the conversation model

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.

Comment 26

[Dan Mosedale (:dmosedale, :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.

Comment 27

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

PR 7 landed https://github.com/mozilla/loop-client/commit/8e6b30668c7e13b3a932c45d28c4fec4b1f5a14b

Will chat with Standard8 about next steps in gecko-dev.

Comment 28

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: [checked in] https://github.com/adamroach/gecko-dev/pull/9

This patch reflects the changes made in loop-client PR 7 to the Desktop client.

Comment 29

[Mark Banner (: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.

Comment 30

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

PR 9 landed on loop-ui-initial https://github.com/adamroach/gecko-dev/commit/52d4dba8bd198cf43de12873eb2276273871cc01

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

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 32

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Done.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 34

[Mark Banner (:standard8)]

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

Comment 35

[Mark Banner (:standard8)]

Attachment: [checked in] Get the basic call working

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.

Comment 36

[Mark Banner (:standard8)]

(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 37

[Dan Mosedale (:dmosedale, :dmose)]

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>.

Comment 38

[Mark Banner (:standard8)]

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

Comment 40

[Mark Banner (:standard8)]

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.

Comment 41

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/9c1eac77ef50
https://hg.mozilla.org/projects/elm/rev/dcfe68e2cdd2
https://hg.mozilla.org/projects/elm/rev/70b1790dce4b
https://hg.mozilla.org/projects/elm/rev/5176bfeba30b
https://hg.mozilla.org/projects/elm/rev/7f1004fbff02
https://hg.mozilla.org/projects/elm/rev/3a2a6ab31822

Comment 42

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/9c1eac77ef50
https://hg.mozilla.org/mozilla-central/rev/dcfe68e2cdd2
https://hg.mozilla.org/mozilla-central/rev/70b1790dce4b
https://hg.mozilla.org/mozilla-central/rev/5176bfeba30b
https://hg.mozilla.org/mozilla-central/rev/7f1004fbff02
https://hg.mozilla.org/mozilla-central/rev/3a2a6ab31822

Comment 43

[u279076]

Marking verified fixed from previous smoketesting.