To start the text-chat work, we want to build a basic working "prototype" that is capable of doing simple text chat. The basic idea here is simple text in text boxes, running over data channels. We might not land it - depending on what state the prototype is in.

I think this is almost ready for review now. I'll take another look through it tomorrow, but the fundamentals are here. The layout is wacky, but it is intended that way until we do the layout properly in other bugs.

Now ready for review. This is the initial version, which is preffed off by default, but intended as something we can build on. The data channels are set up, and there's a basic text chat view to allow chat to be tested. We'll need to do more things, like providing a bit of a protocol for the data channels, as well as all the UX work, but that can build on top of this starter patch.

Comment on attachment 8608057 [details] [diff] [review] Implement an initial layer of text chat for Loop conversations. Review of attachment 8608057 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! :-) So we can argue about the pref name on IRC, but the PropTypes vs. propTypes does need to be fixed, I think. The rest is all nits... ::: browser/app/profile/firefox.js @@ +1698,5 @@ > pref("image.mem.max_decoded_image_kb", 256000); > > pref("loop.enabled", true); > +pref("loop.contextInConversations.enabled", true); > +pref("loop.textChatInConversations.enabled", false); Why not make it 'loop.textChat.enabled'? I don't see any other place for it than conversations, so it's kinda implicit... ::: browser/components/loop/content/js/conversation.jsx @@ +97,5 @@ > var dispatcher = new loop.Dispatcher(); > var client = new loop.Client(); > var sdkDriver = new loop.OTSdkDriver({ > isDesktop: true, > + dataChannelsWanted: dataChannelsWanted, nit: `useDataChannels`? @@ +165,5 @@ > > React.render(<AppControllerView > roomStore={roomStore} > dispatcher={dispatcher} > + mozLoop={navigator.mozLoop} />, document.querySelector('#main')); While you're here, can you change these single quotes to doubles? ::: browser/components/loop/content/shared/css/conversation.css @@ +689,5 @@ > .conversation { > height: 100%; > } > > +.fx-embedded .text-chat-view { Please add a comment that all these text-chat styles are _very_ temporary. ::: browser/components/loop/content/shared/js/actions.js @@ +176,5 @@ > + /** > + * Used to send a message to the other peer. > + */ > + SendTextChatMessage: Action.define("sendTextChatMessage", { > + message: String Can we make this an Object right away, or do you think that's too premature? ::: browser/components/loop/content/shared/js/otSdkDriver.js @@ +409,5 @@ > } > this.connections[connection.id] = connection; > this._notifyMetricsEvent("Session.connectionCreated", "peer"); > this.dispatcher.dispatch(new sharedActions.RemotePeerConnected()); > + whoops, newline. @@ +513,5 @@ > > var remoteElement = this.getScreenShareElementFunc(); > > this.session.subscribe(stream, > + remoteElement, this._getCopyPublisherConfig); AH! This was a bug? ::: browser/components/loop/content/shared/js/textChatStore.js @@ +93,5 @@ > + type: CHAT_MESSAGE_TYPES.SENT, > + message: actionData.message > + }); > + this._sdkDriver.sendTextChatMessage(actionData.message); > + this.setStoreState({messageList: newList}); nit: spaces inside curlies. ::: browser/components/loop/content/shared/js/textChatView.jsx @@ +14,5 @@ > + */ > + var TextChatEntry = React.createClass({ > + mixins: [React.addons.PureRenderMixin], > + > + PropTypes: { Isn't this supposed to be `propTypes`? @@ +123,5 @@ > + message: this.state.messageDetail > + })); > + > + // Reset the form to empty, ready for the next message. > + this.setState({messageDetail: ""}); nit: spaces inside the curlies. ::: browser/components/loop/standalone/content/css/webapp.css @@ +291,5 @@ > /* Hide local media stream when feedback form is shown. */ > display: none; > } > > +.text-chat-view { Same comment here about the temp-ness. ::: browser/components/loop/test/shared/textChatStore_test.js @@ +1,3 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + nit: one newline too many. @@ +66,5 @@ > + sinon.assert.calledWithExactly(fakeSdkDriver.sendTextChatMessage, message); > + }); > + > + it("should add the message to the list", function() { > + var message = "Its awesome!"; nit: s/Its/It's/ ::: browser/components/loop/test/shared/textChatView_test.js @@ +44,5 @@ > + })); > + } > + > + beforeEach(function() { > + store.setStoreState({textChatEnabled: true}); nit: spaces inside curlies throughout this file. @@ +55,5 @@ > + > + expect(view.getDOMNode()).eql(null); > + }); > + > + it("should dispatch `sendTextChatMessage` when enter is pressed", function() { s/`sendTextChatMessage`/SendTextChatMessage action/

(In reply to Mike de Boer [:mikedeboer] from comment #3) > ::: browser/components/loop/content/shared/js/actions.js > @@ +176,5 @@ > > + /** > > + * Used to send a message to the other peer. > > + */ > > + SendTextChatMessage: Action.define("sendTextChatMessage", { > > + message: String > > Can we make this an Object right away, or do you think that's too premature? I was expecting it to be a bit more complicated to work out, but I think its probably reasonable to do now. I've not quite made the action an object, but what we send and receive is now an object. > @@ +513,5 @@ > > > > var remoteElement = this.getScreenShareElementFunc(); > > > > this.session.subscribe(stream, > > + remoteElement, this._getCopyPublisherConfig); > > AH! This was a bug? Not really, but I thought it was cleaner switching to a getter as it matches the data channel one then. > ::: browser/components/loop/content/shared/js/textChatStore.js > @@ +93,5 @@ > > + type: CHAT_MESSAGE_TYPES.SENT, > > + message: actionData.message > > + }); > > + this._sdkDriver.sendTextChatMessage(actionData.message); > > + this.setStoreState({messageList: newList}); > > nit: spaces inside curlies. I don't think we've historically been consistent about that. I think I marginally prefer without, but I've added them anyway.

Updated for review comments.

Comment on attachment 8608655 [details] [diff] [review] Implement an initial layer of text chat for Loop conversations. Review of attachment 8608655 [details] [diff] [review]: ----------------------------------------------------------------- Ship it! ::: browser/app/profile/firefox.js @@ +1703,5 @@ > // (This is intentionally on the high side; see bug 746055.) > pref("image.mem.max_decoded_image_kb", 256000); > > pref("loop.enabled", true); > +pref("loop.contextInConversations.enabled", true); nit: I'm not sure why you chose to move this up here. Preferably I'd like to keep everything where it is.