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.

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.

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] [checked in] Basic conversation window and calls request r=dmose, with the tweaks suggested in the github PR reviewed and the commits squashed.

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] [checked in] Basic conversation window and calls request https://github.com/adamroach/gecko-dev/commit/6ce0fb6b2461f495d24e70edaac7dcb859ec5c78

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.

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

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] > 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] [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

Updated shared assets layout to ease sharing with desktop client.

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.

Comment on attachment 8399452 [details] [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.

PR 7 landed https://github.com/mozilla/loop-client/commit/8e6b30668c7e13b3a932c45d28c4fec4b1f5a14b Will chat with Standard8 about next steps in gecko-dev.

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

Comment on attachment 8400046 [details] [checked in] https://github.com/adamroach/gecko-dev/pull/9 Looks good, r=Standard8. Please squash commits & add bug number when merging.

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

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?

Done.

Comment on attachment 8399632 [details] [checked in] Handle incoming and outgoing calls in the conversation model r=dmose with the details in the PR addressed/responded to

Comment on attachment 8399632 [details] [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

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.

(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] [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 on attachment 8400187 [details] [checked in] Get the basic call working https://github.com/adamroach/gecko-dev/commit/fd16e4fe4ede324a205b5bfa35738695da4982be https://github.com/adamroach/gecko-dev/commit/2be32f330e6eeea9643ef8f1f66bdfadc03bb833

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.

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

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

Marking verified fixed from previous smoketesting.