Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399253002:
CastExtension Impl for Chromoting Host (Closed)
|
Created:
6 years, 5 months ago by aiguha Modified:
6 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chromoting-reviews_chromium.org, Sergey Ulanov, Wez Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCastExtension Impl for Chromoting Host
Implements the HostExtension[Session] interfaces for WebRTC Peer Connection
support in the Chromoting host.
The CastExtension represents the "casting" capability of the Chromoting
host. The CastExtensionSession encapsulates the WebRTC PeerConnection
architecture needed to set up a peer connection with the client, using
protocol::ExtensionMessages as the signaling channel for the connection.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290129
Patch Set 1 #Patch Set 2 : Lint fixes and more #Patch Set 3 : Minor Fix #Patch Set 4 : Per-Session Cast Mode #Patch Set 5 : Better creation flow and error-handling #Patch Set 6 : lint fixes #Patch Set 7 : Minor fix #
Total comments: 2
Patch Set 8 : Incorporated New HostExtension Flow #
Total comments: 128
Patch Set 9 : Changes based on review #
Total comments: 29
Patch Set 10 : Rebased for Video Capturer Stuff + Review Comments #
Total comments: 22
Patch Set 11 : Fixes after review #Patch Set 12 : Removed leftover comment #
Total comments: 1
Patch Set 13 : Final Fixes #
Messages
Total messages: 12 (0 generated)
This CL contains the HostExtension and HostExtensionSession impl for WebRTC support in the chromoting host. sergeyu@ suggested you would be the best person to review it. Since it's larger than ideal, I would be happy to schedule a quick face-to-face review, if that works for you. Thanks! https://codereview.chromium.org/399253002/diff/120001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/120001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:440: // new CastVideoCapturer(capture_task_runner_, screen_capturer.Pass())); This is the only bit of code that relies on the other CL linked in the description. I've tried to separate this as best as possible, but this was unavoidable :( I'm confident that we can get the CastVideoCapturer CL in soon, after which I can merge those changes in here. https://codereview.chromium.org/399253002/diff/120001/remoting/host/client_se... File remoting/host/client_session.h (right): https://codereview.chromium.org/399253002/diff/120001/remoting/host/client_se... remoting/host/client_session.h:166: scoped_ptr<webrtc::ScreenCapturer> RequestScreenCapturer(); The actual implementation of this hook is in https://codereview.chromium.org/398873005/. The declaration is included here based on advice in that CL's review as well. This hook is the current working solution to sharing the ScreenCapturer with the ExtensionSession, however wez@, sergeyu@ and I have discussed another solution offline that he is working on. In the meantime, I'd like to discuss & solve all other issues with this CL so it is ready when the other CL lands.
PTAL: Incorporated wez@'s changes to HostExtension mechanism and setup CastExtensionSession to create it's own worker thread so that we need not change chromoting_host_context.cc at all. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:15: // #include "remoting/host/cast_video_capturer.h" As mentioned earlier: The CastVideoCapturer CL should be in soon so I'll be able to merge that in.
Apologies for the delay in reviewing this; initial comments below - based on our discussion today I'll hold of further review til the new changes are in. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.cc:30: return std::string(kCapability); Can't you just: return kCapability ? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:11: #include "net/url_request/url_request_context_getter.h" Forward define this rather than include? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:33: virtual std::string capability() const OVERRIDE; nit: Suggest comment "HostExtension interface." to precede these. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:39: // Parameters passed to CastExtensionSession on creation. nit: This comment is not necessary, since there are no other members to distinguish these from. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:43: }; DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:15: // #include "remoting/host/cast_video_capturer.h" On 2014/08/04 17:45:36, aiguha wrote: > As mentioned earlier: > The CastVideoCapturer CL should be in soon so I'll be able to merge that in. when you have dependent CLs, just keep them in dependent branches locally, and submit the CLs for review as-is - no need to comment out lines like this - it's not like this code will compile without this header anyway. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:29: const char kMessageData[] = "data"; // Use chromoting_data for CCv2. What is CCv2? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:35: // const char kSubjectSDP[] = "webrtc_sdp"; Why is this commented out? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:40: const char kWebRtcPrefix[] = "webrtc_"; What's this used for? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:42: // WebRTC Headers inside cast extension messages. s/Headers/headers https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:49: // Constants used by PeerConnection. You mean "over the PeerConnection"? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:52: const char kDefaultStunURI[] = "stun:stun.l.google.com:19302"; Where does this come from? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:58: //------------------------------------------------------------------------------ Suggest getting rid of this separator comment. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:60: // webrtc::SetSessionDescriptionObserver implementation. Suggest replacing this with a short description of why we need to implement that interface. :) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:68: VLOG(1) << "SetSessionDescriptionObserver success."; Don't you mean "SetSessionDescription succeeded."? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:72: << error; And "SetSessionDescription failed: <error>"? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:78: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:91: LOG(ERROR) << "No Session, cannot create session description."; How can session_ ever be NULL? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:111: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:122: if (reports.empty()) { Remove this special-case. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:136: void LogStatsReport(const webrtc::StatsReport& report) { This seems simple enough to leave in-line on OnComplete. You're logging each parameter on its own line; consider condensing your logging so it's of the form "<name>=<value>, <name>=<value>..." so that you can have one report per-line. If there are too many params for that to look good then just log a leader line for each StatsReport to separate it from the previous one. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:137: typedef webrtc::StatsReport StatsReport; You don't use this typedef? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:144: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:146: //------------------------------------------------------------------------------ No need for this. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:149: CastExtensionSession* CastExtensionSession::Create( Generally speaking it's best if you can order the implementations of methods the same as their declaration order in the header. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:162: success = cast_extension_session->InitializePeerConnection(); You're overwriting the result of WrapTasksAndSave(). Why not early-exit on failures rather than trying to keep a success boolean? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:182: DCHECK(network_task_runner_); nit: if you're going to -> this on the next line, there's no need to DCHECK it https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:188: // Launch the worker thread. nit: This comment adds nothing ;) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:191: worker_thread_->StartWithType(base::MessageLoop::TYPE_IO); AutoThread is really intended to be created with AutoThread::Create(), and passed an underlying AutoThreadTaskRunner to join() on when it's no longer required. Since you're not making use of that capability, you should probably just use base::Thread. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:198: VLOG(1) << "CastExtensionSession::DTOR"; nit: Clean up this logging. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:203: std::string append_path(const char* first, const char* second) { AppendPath https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:214: std::string sdp; Why not just pull out the kMessageData dictionary and then read the fields, removing the need for append_path()? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:280: if (!message.has_type() || message.type().compare(kMessageType) != 0) { Doesn't the HostExtensionSessionManager already check that there is a has_type()? Why not message.type() == kMessageType? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:17: #include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h" Forward define? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:20: #include "third_party/libjingle/source/talk/media/base/videocapturer.h" Do you need this or could you forward-declare here? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:40: // the PeerConnection native APIs. Suggest "HostExtensionSession implementation that...." https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:46: // Creates and returns a CastExtensionSession object, after performing nit: Blank line between dtor and comment. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:49: static CastExtensionSession* Create( scoped_ptr<CastExtensionSession> https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:50: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner, Is this TaskRunner used for WebRTC networking, or is it really the "caller" TaskRunner? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:56: // HostExtensionSession implementation. nit: Elsewhere in remoting we use "<Foo> interface." not "<Foo> implementation." https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:63: scoped_ptr<webrtc::ScreenCapturer> capturer) OVERRIDE; nit: It's usually a good idea to list interface overrides in the same order as in the interface's declaration. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:65: // PeerConnectionObserver implementation. nit: You mean webrtc::PeerConnectionObserver? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:84: // MouseShapeObserver implementation. Suggest moving this interface definition up nearer the HostExtensionSession interface, to keep the remoting:: and talk_base::/webrtc:: interface overrides grouped together. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:90: void OnFailure(const std::string& error); So are they part of some specific interface? Or do we Bind() them into callbacks somewhere? If the latter then I suggest putting them at the top, following the Create() method, so that all the class-specific methods are grouped ahead of the interface overrides. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:90: void OnFailure(const std::string& error); Suggest renaming these "OnSessionDescription" and "OnSessionDescriptionError". You might also make them private. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:102: bool ParseAndSetRemoteDescription(base::DictionaryValue* message); Could this be a const base::DictionaryValue&, here and below? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:108: // Sends messages to client using the ClientStub through |client_session_|. s/messages/a message? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:108: // Sends messages to client using the ClientStub through |client_session_|. you don't have |client_session_| any more, so I think you mean "to the client through |client_stub_|"? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // network thread. s/network/caller https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // network thread. What are the threading requirements of all the other threads? Let's discuss. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:111: bool SendMessageToClient(const char* subject, const std::string& data); What does the |subject| parameter mean? Why is it const char* rather than const std::string&? Why does |data| need to be JSON formatted? Aren't extension messages opaque strings? If you want |data| to be formatted from JSON then why not have the caller pass in a base::Value and serialize it in the Send method, so it's guaranteed to output JSON? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:114: // |client_session_|. Must also be called on the caller thread? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:119: // is not NULL, signals the event on completion. Why would you ever not want to wait for completion? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:132: // Sets |this| as the MouseShapeObserver of |screen_capturer|. Constructs a How is becoming the MouseShapeObserver part of setting up the video stream? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:145: // NULL. They are all ref-counted, though, so this won't necessarily delete them, just release our references to them? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:153: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; Suggest keeping the network and worker task runners near each other, for clarity. https://codereview.chromium.org/399253002/diff/140001/remoting/host/client_se... File remoting/host/client_session.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/client_se... remoting/host/client_session.h:160: scoped_ptr<webrtc::ScreenCapturer> RequestScreenCapturer(); You don't need this any more.
PTAL https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.cc:30: return std::string(kCapability); On 2014/08/07 01:25:29, Wez wrote: > Can't you just: > > return kCapability > > ? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:11: #include "net/url_request/url_request_context_getter.h" On 2014/08/07 01:25:29, Wez wrote: > Forward define this rather than include? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:33: virtual std::string capability() const OVERRIDE; On 2014/08/07 01:25:29, Wez wrote: > nit: Suggest comment "HostExtension interface." to precede these. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:39: // Parameters passed to CastExtensionSession on creation. On 2014/08/07 01:25:29, Wez wrote: > nit: This comment is not necessary, since there are no other members to > distinguish these from. Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.h:43: }; On 2014/08/07 01:25:29, Wez wrote: > DISALLOW_COPY_AND_ASSIGN(...) Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:15: // #include "remoting/host/cast_video_capturer.h" On 2014/08/07 01:25:32, Wez wrote: > On 2014/08/04 17:45:36, aiguha wrote: > > As mentioned earlier: > > The CastVideoCapturer CL should be in soon so I'll be able to merge that in. > > when you have dependent CLs, just keep them in dependent branches locally, and > submit the CLs for review as-is - no need to comment out lines like this - it's > not like this code will compile without this header anyway. Thanks for the advice. Done! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:29: const char kMessageData[] = "data"; // Use chromoting_data for CCv2. On 2014/08/07 01:25:33, Wez wrote: > What is CCv2? This comment shouldn't have made it in, I'll remove it. Just an internal reminder that I had changed the string to chromoting_data to be less generic (and to avoid confusion with the "data" header that the Chromecast SDK uses). https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:35: // const char kSubjectSDP[] = "webrtc_sdp"; On 2014/08/07 01:25:31, Wez wrote: > Why is this commented out? Not used anymore, removed! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:40: const char kWebRtcPrefix[] = "webrtc_"; On 2014/08/07 01:25:31, Wez wrote: > What's this used for? It was being used to construct the subject of a message, by prepending it to desc->type() where desc is webrtc::SessionDescription, which the webrtc internals generates. See OnCreateSessionDescription(). I've changed it to avoid using this class member. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:42: // WebRTC Headers inside cast extension messages. On 2014/08/07 01:25:30, Wez wrote: > s/Headers/headers Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:49: // Constants used by PeerConnection. On 2014/08/07 01:25:33, Wez wrote: > You mean "over the PeerConnection"? The constants are used by the related objects, but I guess "over" makes more sense. Changed. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:52: const char kDefaultStunURI[] = "stun:stun.l.google.com:19302"; On 2014/08/07 01:25:33, Wez wrote: > Where does this come from? I believe this is Google's public STUN server. The webrtc demo uses the same one. Ideally, we'd have multiple STUN and TURN servers (but I had no user/pass credentials for the latter). https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:58: //------------------------------------------------------------------------------ On 2014/08/07 01:25:30, Wez wrote: > Suggest getting rid of this separator comment. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:60: // webrtc::SetSessionDescriptionObserver implementation. On 2014/08/07 01:25:30, Wez wrote: > Suggest replacing this with a short description of why we need to implement that > interface. :) Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:68: VLOG(1) << "SetSessionDescriptionObserver success."; On 2014/08/07 01:25:32, Wez wrote: > Don't you mean "SetSessionDescription succeeded."? Yes, changed! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:72: << error; On 2014/08/07 01:25:33, Wez wrote: > And "SetSessionDescription failed: <error>"? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:78: }; On 2014/08/07 01:25:32, Wez wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:91: LOG(ERROR) << "No Session, cannot create session description."; On 2014/08/07 01:25:33, Wez wrote: > How can session_ ever be NULL? Since this is ref-counted and owned by PeerConnection, couldn't it outlive session_, in which case this would be NULL? Should I use a WeakPtr instead? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:111: }; On 2014/08/07 01:25:32, Wez wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:122: if (reports.empty()) { On 2014/08/07 01:25:30, Wez wrote: > Remove this special-case. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:136: void LogStatsReport(const webrtc::StatsReport& report) { On 2014/08/07 01:25:30, Wez wrote: > This seems simple enough to leave in-line on OnComplete. > > You're logging each parameter on its own line; consider condensing your logging > so it's of the form "<name>=<value>, <name>=<value>..." so that you can have one > report per-line. If there are too many params for that to look good then just > log a leader line for each StatsReport to separate it from the previous one. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:137: typedef webrtc::StatsReport StatsReport; On 2014/08/07 01:25:32, Wez wrote: > You don't use this typedef? Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:144: }; On 2014/08/07 01:25:31, Wez wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:146: //------------------------------------------------------------------------------ On 2014/08/07 01:25:30, Wez wrote: > No need for this. Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:149: CastExtensionSession* CastExtensionSession::Create( On 2014/08/07 01:25:31, Wez wrote: > Generally speaking it's best if you can order the implementations of methods the > same as their declaration order in the header. Done. Only the PeerConnectionObserver impl is still at the bottom because it's trivial and fairly long. If you still think it should be in order, I'll move it as well :) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:162: success = cast_extension_session->InitializePeerConnection(); On 2014/08/07 01:25:31, Wez wrote: > You're overwriting the result of WrapTasksAndSave(). > > Why not early-exit on failures rather than trying to keep a success boolean? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:182: DCHECK(network_task_runner_); On 2014/08/07 01:25:31, Wez wrote: > nit: if you're going to -> this on the next line, there's no need to DCHECK it Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:188: // Launch the worker thread. On 2014/08/07 01:25:33, Wez wrote: > nit: This comment adds nothing ;) Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:191: worker_thread_->StartWithType(base::MessageLoop::TYPE_IO); On 2014/08/07 01:25:32, Wez wrote: > AutoThread is really intended to be created with AutoThread::Create(), and > passed an underlying AutoThreadTaskRunner to join() on when it's no longer > required. > > Since you're not making use of that capability, you should probably just use > base::Thread. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:198: VLOG(1) << "CastExtensionSession::DTOR"; On 2014/08/07 01:25:32, Wez wrote: > nit: Clean up this logging. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:203: std::string append_path(const char* first, const char* second) { On 2014/08/07 01:25:30, Wez wrote: > AppendPath Removed altogether. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:214: std::string sdp; On 2014/08/07 01:25:32, Wez wrote: > Why not just pull out the kMessageData dictionary and then read the fields, > removing the need for append_path()? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:280: if (!message.has_type() || message.type().compare(kMessageType) != 0) { On 2014/08/07 01:25:33, Wez wrote: > Doesn't the HostExtensionSessionManager already check that there is a > has_type()? > > Why not message.type() == kMessageType? That was my Java mind talking. Fixed! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:280: if (!message.has_type() || message.type().compare(kMessageType) != 0) { On 2014/08/07 01:25:33, Wez wrote: > Doesn't the HostExtensionSessionManager already check that there is a > has_type()? > > Why not message.type() == kMessageType? Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:17: #include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h" On 2014/08/07 01:25:35, Wez wrote: > Forward define? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:20: #include "third_party/libjingle/source/talk/media/base/videocapturer.h" On 2014/08/07 01:25:35, Wez wrote: > Do you need this or could you forward-declare here? Left-over include actually. Removed! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:40: // the PeerConnection native APIs. On 2014/08/07 01:25:34, Wez wrote: > Suggest "HostExtensionSession implementation that...." Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:46: // Creates and returns a CastExtensionSession object, after performing On 2014/08/07 01:25:35, Wez wrote: > nit: Blank line between dtor and comment. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:49: static CastExtensionSession* Create( On 2014/08/07 01:25:35, Wez wrote: > scoped_ptr<CastExtensionSession> I thought returning a scoped_ptr would be better too, but I was following some other factory method examples I'd seen. Anyway, changed it to return scoped_ptr. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:50: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner, On 2014/08/07 01:25:36, Wez wrote: > Is this TaskRunner used for WebRTC networking, or is it really the "caller" > TaskRunner? This TaskRunner is used to send/receive ExtensionMessages to/from the client. Its thread is also wrapped to an rtc::Thread and used as the signaling thread. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:56: // HostExtensionSession implementation. On 2014/08/07 01:25:35, Wez wrote: > nit: Elsewhere in remoting we use "<Foo> interface." not "<Foo> implementation." Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:63: scoped_ptr<webrtc::ScreenCapturer> capturer) OVERRIDE; On 2014/08/07 01:25:34, Wez wrote: > nit: It's usually a good idea to list interface overrides in the same order as > in the interface's declaration. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:65: // PeerConnectionObserver implementation. On 2014/08/07 01:25:35, Wez wrote: > nit: You mean webrtc::PeerConnectionObserver? Yes, my mistake. Fixed! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:84: // MouseShapeObserver implementation. On 2014/08/07 01:25:34, Wez wrote: > Suggest moving this interface definition up nearer the HostExtensionSession > interface, to keep the remoting:: and talk_base::/webrtc:: interface overrides > grouped together. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:90: void OnFailure(const std::string& error); On 2014/08/07 01:25:35, Wez wrote: > Suggest renaming these "OnSessionDescription" and "OnSessionDescriptionError". > You might also make them private. Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:90: void OnFailure(const std::string& error); On 2014/08/07 01:25:35, Wez wrote: > So are they part of some specific interface? Or do we Bind() them into callbacks > somewhere? If the latter then I suggest putting them at the top, following the > Create() method, so that all the class-specific methods are grouped ahead of the > interface overrides. They're not part of an interface, but callbacks (called directly, not binded) that the nested class impl of webrtc::CreateSessionDescriptionObserver call. I used a nested class because webrtc::CreateSessionDescriptionObserver implements rtc::RefCountInterface, but CastExtensionSession itself can't be ref counted. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:102: bool ParseAndSetRemoteDescription(base::DictionaryValue* message); On 2014/08/07 01:25:35, Wez wrote: > Could this be a const base::DictionaryValue&, here and below? No, because we use GetDictionary() which is not marked const. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:108: // Sends messages to client using the ClientStub through |client_session_|. On 2014/08/07 01:25:35, Wez wrote: > s/messages/a message? Done. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:108: // Sends messages to client using the ClientStub through |client_session_|. On 2014/08/07 01:25:36, Wez wrote: > you don't have |client_session_| any more, so I think you mean "to the client > through |client_stub_|"? Yes, forgot to update the comment, thanks! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // network thread. On 2014/08/07 01:25:36, Wez wrote: > What are the threading requirements of all the other threads? > > Let's discuss. The only threading requirements are that chromoting-host-facing methods (SendMessageToClient(), SendCursorShape(), OnExtensionMessage()) need to happen on the network (i.e., signaling)) thread. Sure, we can talk offline today if you'd like. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:111: bool SendMessageToClient(const char* subject, const std::string& data); On 2014/08/07 01:25:34, Wez wrote: > What does the |subject| parameter mean? Why is it const char* rather than const > std::string&? > Why does |data| need to be JSON formatted? Aren't extension messages opaque > strings? If you want |data| to be formatted from JSON then why not have the > caller pass in a base::Value and serialize it in the Send method, so it's > guaranteed to output JSON? It's now std::string&, thanks for pointing that out. The |subject| parameter is just part of a little sub-specification I created for the Cast-communication. We use the ExtensionMessages to send both control and WebRTC session negotiation messages. The |subject| specifies the message's "subject". The host and client use the same list of subjects to specify the communication. (I'll be documenting this). https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:114: // |client_session_|. On 2014/08/07 01:25:34, Wez wrote: > Must also be called on the caller thread? Yes, will update! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:119: // is not NULL, signals the event on completion. On 2014/08/07 01:25:35, Wez wrote: > Why would you ever not want to wait for completion? We need to EnsureTaskAndSetSend() for both threads. We first call it on the current thread, so we don't need to be notified because it's synchronous. We then PostTask this on the other thread, so pass in |event| to be notified when it completes. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:132: // Sets |this| as the MouseShapeObserver of |screen_capturer|. Constructs a On 2014/08/07 01:25:34, Wez wrote: > How is becoming the MouseShapeObserver part of setting up the video stream? SetupVideoStream() after we successfully grab the ScreenCapturer in OnCreateVideoCapturer(). But I agree, I'll move the line of code to OnCreateVideoCapturer() itself. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:145: // NULL. On 2014/08/07 01:25:34, Wez wrote: > They are all ref-counted, though, so this won't necessarily delete them, just > release our references to them? Agreed. Rename to CleanupPeerConnection()? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:153: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; On 2014/08/07 01:25:34, Wez wrote: > Suggest keeping the network and worker task runners near each other, for > clarity. Done. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:153: // |peer_conn_factory_|). For more details, see bug:. Filing the bug shortly.
https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.cc:7: #include <string> nit: Strictly you should keep the #include <string>, since you're returning a std::string. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:49: // Constants used by PeerConnection. On 2014/08/12 01:42:36, aiguha wrote: > On 2014/08/07 01:25:33, Wez wrote: > > You mean "over the PeerConnection"? > > The constants are used by the related objects, but I guess "over" makes more > sense. Changed. Well, it depends - are these things that get sent over the connection, or constants used to configure objects, or something else? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:91: LOG(ERROR) << "No Session, cannot create session description."; On 2014/08/12 01:42:37, aiguha wrote: > On 2014/08/07 01:25:33, Wez wrote: > > How can session_ ever be NULL? > > Since this is ref-counted and owned by PeerConnection, couldn't it outlive > session_, in which case this would be NULL? Should I use a WeakPtr instead? Nothing ever sets it to NULL, though...? If |session_| gets deleted while this object is still active then it'll end up a dangling point to dead memory - you need to either clear it explicitly, ideally, or use a WeakPtr if for some reason that's not feasible. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:49: static CastExtensionSession* Create( On 2014/08/12 01:42:40, aiguha wrote: > On 2014/08/07 01:25:35, Wez wrote: > > scoped_ptr<CastExtensionSession> > > I thought returning a scoped_ptr would be better too, but I was following some > other factory method examples I'd seen. Anyway, changed it to return scoped_ptr. Those probably just pre-date scoped_ptr<> being supported as a return-value! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:50: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner, On 2014/08/12 01:42:39, aiguha wrote: > On 2014/08/07 01:25:36, Wez wrote: > > Is this TaskRunner used for WebRTC networking, or is it really the "caller" > > TaskRunner? > > This TaskRunner is used to send/receive ExtensionMessages to/from the client. That happens via the ClientStub, right? So from this class' point of view, it's the caller's thread? > Its thread is also wrapped to an rtc::Thread and used as the signaling thread. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // network thread. On 2014/08/12 01:42:39, aiguha wrote: > On 2014/08/07 01:25:36, Wez wrote: > > What are the threading requirements of all the other threads? > > > > Let's discuss. > > The only threading requirements are that chromoting-host-facing methods > (SendMessageToClient(), SendCursorShape(), OnExtensionMessage()) need to happen > on the network (i.e., signaling)) thread. Sure, we can talk offline today if > you'd like. This extension session object is an "owned" object, though, so there must be some expectations and requirements on which methods are accessed from which threads, surely? https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:111: bool SendMessageToClient(const char* subject, const std::string& data); On 2014/08/12 01:42:41, aiguha wrote: > On 2014/08/07 01:25:34, Wez wrote: > > What does the |subject| parameter mean? Why is it const char* rather than > const > > std::string&? > > Why does |data| need to be JSON formatted? Aren't extension messages opaque > > strings? If you want |data| to be formatted from JSON then why not have the > > caller pass in a base::Value and serialize it in the Send method, so it's > > guaranteed to output JSON? > > It's now std::string&, thanks for pointing that out. > > The |subject| parameter is just part of a little sub-specification I created for > the Cast-communication. We use the ExtensionMessages to send both control and > WebRTC session negotiation messages. The |subject| specifies the message's > "subject". The host and client use the same list of subjects to specify the > communication. (I'll be documenting this). OK; I think the comment here should clarify what the |subject| means (beyond saying it specifies the "subject" ;) https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:30: const char kMessageData[] = "chromoting_data"; These two names are confusing; suggest kCastMessageType and kExtensionMessageType, i.e. make clear which of these are for the Cast messages, and which are for the Extension messages. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:58: // Minimum frame rate for video streaming over the PeerConnection. What are the units? Why is this a char rather than an int? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:182: webrtc::SessionDescriptionInterface* desc) { nit: Separate out the logical chunks of code in this function with blank lines, e.g. the thread-hop is logically separate from setting the local description, etc. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:216: // peer connection in the OnRenegotiationNeeded() callback. nit: File a bug for that work and refer to it here. :) https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:246: // Returns true if the ExtensionMessage was of type |kMessageType|, even if nit: You mean if its a Cast extension message? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:292: void CastExtensionSession::OnCursorShapeChanged( I can find a call to SetMouseShapeObserver in this class, and in any case we're removing that in favour of MouseCursorMonitor, so suggest removing cursor shape handling for now and filing a bug to re-add it. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:336: base::Thread::Options options(base::MessageLoop::TYPE_IO, 0); nit: You could clarify why this needs to be an IO thread, if you want. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:511: webrtc::FakeConstraints constraints; Why are you using FakeConstraints here? Fakes are normally only for testing purposes. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:531: if (!SendMessageToClient(kSubjectTest, "Hello, client.") || What purposes does the test message serve? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:567: !peer_connection_->AddStream(stream_, NULL)) nit: This is now a multi-line if() so it needs {} In general you seem to be using {} event for single-line if()s, in fact, so please be consistent. :) https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:72: // webrtc::ScreenCapturer::MouseShapeObserver interface. Hmmm, you shouldn't be using MouseShapeObserver since that's going away; perhaps you mean MouseCursorMonitor::Callback? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:146: // NULL. Rather than "sets to nULL", suggest "releases |peet_conn_factory|" etc
Got the 3 bugs to be filed waiting to be published. I thought I could just show them to you before sending them out. Apart from that everything should be done! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension.cc:7: #include <string> On 2014/08/12 22:14:59, Wez wrote: > nit: Strictly you should keep the #include <string>, since you're returning a > std::string. It's in the .h file :) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:49: // Constants used by PeerConnection. On 2014/08/12 22:14:59, Wez wrote: > On 2014/08/12 01:42:36, aiguha wrote: > > On 2014/08/07 01:25:33, Wez wrote: > > > You mean "over the PeerConnection"? > > > > The constants are used by the related objects, but I guess "over" makes more > > sense. Changed. > > Well, it depends - are these things that get sent over the connection, or > constants used to configure objects, or something else? The labels are used over the connection and the StunURI is used to configure the peer connection. I'll separate them and make things clear, thanks! https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:91: LOG(ERROR) << "No Session, cannot create session description."; On 2014/08/12 22:14:59, Wez wrote: > On 2014/08/12 01:42:37, aiguha wrote: > > On 2014/08/07 01:25:33, Wez wrote: > > > How can session_ ever be NULL? > > > > Since this is ref-counted and owned by PeerConnection, couldn't it outlive > > session_, in which case this would be NULL? Should I use a WeakPtr instead? > > Nothing ever sets it to NULL, though...? > > If |session_| gets deleted while this object is still active then it'll end up a > dangling point to dead memory - you need to either clear it explicitly, ideally, > or use a WeakPtr if for some reason that's not feasible. Oops, I was mixing up dangling and null pointers, ouch. :O Proposed fix: save a reference to this Observer object in CastExtensionSession. In the CastExtensionSession dtor call CastCreateSessionDescriptionObserver::set_extension_session(NULL) to explictly clear the pointer. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:49: static CastExtensionSession* Create( On 2014/08/12 22:14:59, Wez wrote: > On 2014/08/12 01:42:40, aiguha wrote: > > On 2014/08/07 01:25:35, Wez wrote: > > > scoped_ptr<CastExtensionSession> > > > > I thought returning a scoped_ptr would be better too, but I was following some > > other factory method examples I'd seen. Anyway, changed it to return > scoped_ptr. > > Those probably just pre-date scoped_ptr<> being supported as a return-value! Acknowledged. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:50: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner, On 2014/08/12 22:15:00, Wez wrote: > On 2014/08/12 01:42:39, aiguha wrote: > > On 2014/08/07 01:25:36, Wez wrote: > > > Is this TaskRunner used for WebRTC networking, or is it really the "caller" > > > TaskRunner? > > > > This TaskRunner is used to send/receive ExtensionMessages to/from the client. > > That happens via the ClientStub, right? So from this class' point of view, it's > the caller's thread? > > > Its thread is also wrapped to an rtc::Thread and used as the signaling thread. I see your point. Renamed this the caller_task_runner_ and the corresponding thread wrapper is now signaling_thread_wrapper_. https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // network thread. On 2014/08/12 22:14:59, Wez wrote: > On 2014/08/12 01:42:39, aiguha wrote: > > On 2014/08/07 01:25:36, Wez wrote: > > > What are the threading requirements of all the other threads? > > > > > > Let's discuss. > > > > The only threading requirements are that chromoting-host-facing methods > > (SendMessageToClient(), SendCursorShape(), OnExtensionMessage()) need to > happen > > on the network (i.e., signaling)) thread. Sure, we can talk offline today if > > you'd like. > > This extension session object is an "owned" object, though, so there must be > some expectations and requirements on which methods are accessed from which > threads, surely? Yes, we've got the following: network thread (caller): all host-facing methods - ResetVideoPipeline() , DeliverHostMessage() and SetCursorShape()(being removed) - are called on this thread. PeerConnection::Close() should also be called on this thread. worker thread (self-owned): Used internally by PeerConnection. The CastVideoCapturerAdapter's Start() and Stop() will be called on this thread (though that doesn't affect us, as long as it is the same thread). If I'm misunderstanding your question, let's definitely talk tomorrow :) https://codereview.chromium.org/399253002/diff/140001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:111: bool SendMessageToClient(const char* subject, const std::string& data); On 2014/08/12 22:14:59, Wez wrote: > On 2014/08/12 01:42:41, aiguha wrote: > > On 2014/08/07 01:25:34, Wez wrote: > > > What does the |subject| parameter mean? Why is it const char* rather than > > const > > > std::string&? > > > Why does |data| need to be JSON formatted? Aren't extension messages opaque > > > strings? If you want |data| to be formatted from JSON then why not have the > > > caller pass in a base::Value and serialize it in the Send method, so it's > > > guaranteed to output JSON? > > > > It's now std::string&, thanks for pointing that out. > > > > The |subject| parameter is just part of a little sub-specification I created > for > > the Cast-communication. We use the ExtensionMessages to send both control and > > WebRTC session negotiation messages. The |subject| specifies the message's > > "subject". The host and client use the same list of subjects to specify the > > communication. (I'll be documenting this). > > OK; I think the comment here should clarify what the |subject| means (beyond > saying it specifies the "subject" ;) Totally agree! I realized that this is the best place for the documentation of the communication spec, so I've described it completely in comments in the next patch. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:30: const char kMessageData[] = "chromoting_data"; On 2014/08/12 22:15:01, Wez wrote: > These two names are confusing; suggest kCastMessageType and > kExtensionMessageType, i.e. make clear which of these are for the Cast messages, > and which are for the Extension messages. Yes, I can see how that's confusing. kMessageType --> kExtensionMessageType makes sense, done. There's no difference between Cast messages and Extension messages here; we use ExtensionMessages with type=="cast_message" for everything. I've documented how the spec in the .h file. Based on that, I think: kMessageData --> kTopLevelData || kMainMessageData kMessageSubject --> kTopLevelSubject || kMainMessageSubject would make sense. Changed it to the first for now. What do you think? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:58: // Minimum frame rate for video streaming over the PeerConnection. On 2014/08/12 22:15:00, Wez wrote: > What are the units? Why is this a char rather than an int? Clarified in the comment now. It's in fps. We're using this as a video constraint and I assumed AddMandatory() only accepted string keys and values. It's actually templated, so I've made this a simple int. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:182: webrtc::SessionDescriptionInterface* desc) { On 2014/08/12 22:15:00, Wez wrote: > nit: Separate out the logical chunks of code in this function with blank lines, > e.g. the thread-hop is logically separate from setting the local description, > etc. Done. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:216: // peer connection in the OnRenegotiationNeeded() callback. On 2014/08/12 22:15:01, Wez wrote: > nit: File a bug for that work and refer to it here. :) Acknowledged. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:246: // Returns true if the ExtensionMessage was of type |kMessageType|, even if On 2014/08/12 22:15:00, Wez wrote: > nit: You mean if its a Cast extension message? Yes, that would be simpler! https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:292: void CastExtensionSession::OnCursorShapeChanged( On 2014/08/12 22:15:00, Wez wrote: > I can find a call to SetMouseShapeObserver in this class, and in any case we're > removing that in favour of MouseCursorMonitor, so suggest removing cursor shape > handling for now and filing a bug to re-add it. Acknowledged. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:336: base::Thread::Options options(base::MessageLoop::TYPE_IO, 0); On 2014/08/12 22:15:01, Wez wrote: > nit: You could clarify why this needs to be an IO thread, if you want. Done. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:511: webrtc::FakeConstraints constraints; On 2014/08/12 22:15:00, Wez wrote: > Why are you using FakeConstraints here? Fakes are normally only for testing > purposes. The only other existing impl of webrtc::MediaConstraintsInterface is content::RTCMediaConstraints which seemed to serve a slightly different purpose (glue between Webkit media constraints and WebRTC). It also only supports string/string key values, which means kMinFrameRate will have to be a char[] again. The FakeConstraints impl seemed cleaner and simpler. Which one would you prefer? Or should I just duplicate the FakeConstraints implementation as a non-test class? https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:531: if (!SendMessageToClient(kSubjectTest, "Hello, client.") || On 2014/08/12 22:15:00, Wez wrote: > What purposes does the test message serve? The test message always receives an echo from the client, thus testing basic two-way communication. The ready message kick starts the entire webrtc signaling process. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:567: !peer_connection_->AddStream(stream_, NULL)) On 2014/08/12 22:15:00, Wez wrote: > nit: This is now a multi-line if() so it needs {} > > In general you seem to be using {} event for single-line if()s, in fact, so > please be consistent. :) > Absolutely agree, sorry about that! https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:72: // webrtc::ScreenCapturer::MouseShapeObserver interface. On 2014/08/12 22:15:01, Wez wrote: > Hmmm, you shouldn't be using MouseShapeObserver since that's going away; perhaps > you mean MouseCursorMonitor::Callback? Removed for now as suggested in .cc. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:146: // NULL. On 2014/08/12 22:15:01, Wez wrote: > Rather than "sets to nULL", suggest "releases |peet_conn_factory|" etc Done.
https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:30: const char kMessageData[] = "chromoting_data"; On 2014/08/13 18:33:27, aiguha wrote: > On 2014/08/12 22:15:01, Wez wrote: > > These two names are confusing; suggest kCastMessageType and > > kExtensionMessageType, i.e. make clear which of these are for the Cast > messages, > > and which are for the Extension messages. > > Yes, I can see how that's confusing. > kMessageType --> kExtensionMessageType makes sense, done. > > There's no difference between Cast messages and Extension messages here; we use > ExtensionMessages with type=="cast_message" for everything. > > I've documented how the spec in the .h file. Based on that, I think: > kMessageData --> kTopLevelData || kMainMessageData > kMessageSubject --> kTopLevelSubject || kMainMessageSubject > would make sense. Changed it to the first for now. What do you think? Come to think of it, "cast" arguably shouldn't feature in the name, since it has nothing to do with Cast, per-se; this is a WebRTC video extension. Rather than kTopLevel*, what about kSubjectName and kDataName? If you rename Subject to Type then you can have e.g. kReadyType, etc. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:511: webrtc::FakeConstraints constraints; On 2014/08/13 18:33:27, aiguha wrote: > On 2014/08/12 22:15:00, Wez wrote: > > Why are you using FakeConstraints here? Fakes are normally only for testing > > purposes. > > The only other existing impl of webrtc::MediaConstraintsInterface is > content::RTCMediaConstraints which seemed to serve a slightly different purpose > (glue between Webkit media constraints and WebRTC). It also only supports > string/string key values, which means kMinFrameRate will have to be a char[] > again. The FakeConstraints impl seemed cleaner and simpler. > > Which one would you prefer? Or should I just duplicate the FakeConstraints > implementation as a non-test class? Let's use FakeConstraints for now, but add a TODO comment to clean that up. https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:531: if (!SendMessageToClient(kSubjectTest, "Hello, client.") || On 2014/08/13 18:33:27, aiguha wrote: > On 2014/08/12 22:15:00, Wez wrote: > > What purposes does the test message serve? > > The test message always receives an echo from the client, thus testing basic > two-way communication. The ready message kick starts the entire webrtc signaling > process. So... what purpose does the test message serve? Why not just use the channel? What is it that you do with a test message that no other message would achieve? https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:41: // WebRTC headers used inside cast extension messages. Inside messages with subject = webrtc_* ? https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:64: const int kMinFrameRate = 5; nit: kMinimumFps or kMinFramesPerSecond https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:158: // |peer_conn_factory_|). Tracked by: crbug.com/. Bug #? https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:158: // |peer_conn_factory_|). Tracked by: crbug.com/. Suggest: See crbug.com/<number> https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:225: // Tracked by: crbug.com/. nit: See above re "See crbug.com/..." https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:323: // that require it. Should we file a bug against WebRTC for that behaviour? It seems broken... https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // The specifications for Cast Extension Messages are as follows: nit: This line adds nothing. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:116: // {subject: '...', chromoting_data: '...'} Since the data portion is specific to this extension (via the type), why not call this just 'data'? https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:116: // {subject: '...', chromoting_data: '...'} nit: Suggest just documenting what 'subject' and 'data' mean; the example doesn't really add anything. An example of possible 'subject' values, to illustrate that it's effectively identifying the command the peer wants to perform, may help. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:119: // the peer can easily decide what to do next. The |subject| MUST be one of So the Subject is essentially the extension-specific message type? In the VideoFrameRecorderHostExtension we just called that "type". https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:124: // string. Better to say that the type of 'data' depends on the Subject.
Should be ready! https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/160001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:30: const char kMessageData[] = "chromoting_data"; On 2014/08/14 19:22:03, Wez wrote: > On 2014/08/13 18:33:27, aiguha wrote: > > On 2014/08/12 22:15:01, Wez wrote: > > > These two names are confusing; suggest kCastMessageType and > > > kExtensionMessageType, i.e. make clear which of these are for the Cast > > messages, > > > and which are for the Extension messages. > > > > Yes, I can see how that's confusing. > > kMessageType --> kExtensionMessageType makes sense, done. > > > > There's no difference between Cast messages and Extension messages here; we > use > > ExtensionMessages with type=="cast_message" for everything. > > > > I've documented how the spec in the .h file. Based on that, I think: > > kMessageData --> kTopLevelData || kMainMessageData > > kMessageSubject --> kTopLevelSubject || kMainMessageSubject > > would make sense. Changed it to the first for now. What do you think? > > Come to think of it, "cast" arguably shouldn't feature in the name, since it has > nothing to do with Cast, per-se; this is a WebRTC video extension. > > Rather than kTopLevel*, what about kSubjectName and kDataName? > > If you rename Subject to Type then you can have e.g. kReadyType, etc. I originally had it named WebRtcHostExtension[Session] but Sergey and Gary suggested I rename it CastExtension[Session] :D Can we leave it like this for now and do a follow up CL if necessary? Refactoring would be time-consuming at this stage, and straightforward later on (for anyone to do) :) https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:41: // WebRTC headers used inside cast extension messages. On 2014/08/14 19:22:03, Wez wrote: > Inside messages with subject = webrtc_* ? Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:64: const int kMinFrameRate = 5; On 2014/08/14 19:22:03, Wez wrote: > nit: kMinimumFps or kMinFramesPerSecond Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:158: // |peer_conn_factory_|). Tracked by: crbug.com/. On 2014/08/14 19:22:03, Wez wrote: > Suggest: See crbug.com/<number> Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:158: // |peer_conn_factory_|). Tracked by: crbug.com/. On 2014/08/14 19:22:03, Wez wrote: > Bug #? Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:225: // Tracked by: crbug.com/. On 2014/08/14 19:22:03, Wez wrote: > nit: See above re "See crbug.com/..." Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:323: // that require it. On 2014/08/14 19:22:03, Wez wrote: > Should we file a bug against WebRTC for that behaviour? It seems broken... Done and linked. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... File remoting/host/cast_extension_session.h (right): https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:110: // The specifications for Cast Extension Messages are as follows: On 2014/08/14 19:22:03, Wez wrote: > nit: This line adds nothing. Done. https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:116: // {subject: '...', chromoting_data: '...'} On 2014/08/14 19:22:03, Wez wrote: > nit: Suggest just documenting what 'subject' and 'data' mean; the example > doesn't really add anything. An example of possible 'subject' values, to > illustrate that it's effectively identifying the command the peer wants to > perform, may help. I've made the comment clearer. Also added better examples with descriptions :) https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:116: // {subject: '...', chromoting_data: '...'} On 2014/08/14 19:22:03, Wez wrote: > Since the data portion is specific to this extension (via the type), why not > call this just 'data'? It's "chromoting_data" not just "data" because: 1. ExtensionMessages already have a data field 2. Worse, the Cast SDK, when sending messages as JSON, wraps them all in a "data" header It's "subject" not "type" because: 1. ExtensionMessage already have a type field 2. Similarly, WebRTC uses the type header within its offer/answer messages Having distinct names made things easier for me to work with, as well as for readers/maintainers I think :) https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:119: // the peer can easily decide what to do next. The |subject| MUST be one of On 2014/08/14 19:22:03, Wez wrote: > So the Subject is essentially the extension-specific message type? In the > VideoFrameRecorderHostExtension we just called that "type". responded above https://codereview.chromium.org/399253002/diff/180001/remoting/host/cast_exte... remoting/host/cast_extension_session.h:124: // string. On 2014/08/14 19:22:03, Wez wrote: > Better to say that the type of 'data' depends on the Subject. Done.
lgtm https://codereview.chromium.org/399253002/diff/220001/remoting/host/cast_exte... File remoting/host/cast_extension_session.cc (right): https://codereview.chromium.org/399253002/diff/220001/remoting/host/cast_exte... remoting/host/cast_extension_session.cc:168: create_session_desc_observer_->SetCastExtensionSession(NULL); Nit: Change this comment to explain "why?"
The CQ bit was checked by aiguha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiguha@chromium.org/399253002/240001
Message was sent while issue was closed.
Committed patchset #13 (240001) as 290129 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
