Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10538091:
[Chromoting] Make ChromotingHost's dependency on libjingle injected, instead of hard-coded. (Closed)
|
Created:
8 years, 6 months ago by simonmorris Modified:
8 years, 6 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Make ChromotingHost's dependency on libjingle injected, instead of hard-coded.
This will help improve ChromotingHost's unit tests.
BUG=86546
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142473
Patch Set 1 #
Total comments: 36
Patch Set 2 : Review. #Patch Set 3 : Patch for Windows build. #
Total comments: 1
Patch Set 4 : Remove SessionManagerFactory #Patch Set 5 : Fix remoting.gyp. #
Total comments: 24
Patch Set 6 : Review. #
Total comments: 8
Patch Set 7 : Improve comments. #
Total comments: 2
Patch Set 8 : Tweak a comment. #Patch Set 9 : Sync. #Patch Set 10 : Patch for Windows. #
Messages
Total messages: 22 (0 generated)
ptal
Thanks for doing this - should be a big help to us implementing better test coverage. :) http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.cc File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:70: session_manager_factory_(session_manager_factory.release()), release() -> Pass() http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:101: network_settings_, context_->url_request_context_getter()).release()); release() -> Pass() http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host_... File remoting/host/chromoting_host_unittest.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host_... remoting/host/chromoting_host_unittest.cc:100: session_manager_factory.Pass(), Do you need to assign the Mock to a temporary, above? Can't you just pass in the new object directly here? http://codereview.chromium.org/10538091/diff/1/remoting/host/host_mock_objects.h File remoting/host/host_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/host_mock_object... remoting/host/host_mock_objects.h:153: network_settings, url_request_context_getter)); Although it seems a bit pointless, I think this belongs in the .cc file... http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... File remoting/host/jingle_session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:1: #include "remoting/host/jingle_session_manager_factory.h" You're missing a Copyright header. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:22: // Create port allocator and transport factory. nit: Suggest "Create a cricket::PortAllocator that uses Chrome's network stack"? http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:27: bool incoming_only = network_settings.nat_traversal_mode == nit: Suggest comment "Create a libjingle TransportFactory implementation, using the Chrome-based PortAllocator." http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:35: // Create and session manager. nit: Suggest "Create a Jingle-protocol SessionManager that uses the libjingle TransportFactory." http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... File remoting/host/jingle_session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:14: class JingleSessionManagerFactory : public SessionManagerFactory { Add a brief comment explaining what this factory makes. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:17: virtual ~JingleSessionManagerFactory() OVERRIDE; You don't need OVERRIDE on dtors. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:19: virtual scoped_ptr<protocol::SessionManager> MakeSessionManager( nit: CreateSessionManager is more in keeping with *Factory method naming elsewhere in remoting/ http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:22: url_request_context_getter) OVERRIDE; You're missing the disallow-copy-and-assign macro here. http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... remoting/host/plugin/host_script_object.cc:557: // Create the session manager factory. nit: Suggest e.g. "Create a SessionManagerFactory that uses Jingle on Chrome's network stack". http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... remoting/host/plugin/host_script_object.cc:565: session_manager_factory.Pass(), Do you need to assign to a local & Pass(), or could you just pass the new object directly? http://codereview.chromium.org/10538091/diff/1/remoting/host/remoting_me2me_h... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/remoting_me2me_h... remoting/host/remoting_me2me_host.cc:389: session_manager_factory.Pass(), network_settings); Pass the new object directly here? http://codereview.chromium.org/10538091/diff/1/remoting/host/session_manager_... File remoting/host/session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/session_manager_... remoting/host/session_manager_factory.h:6: #define REMOTING_HOST_SESSION_MANAGER_FACTORY_H_ Since this is a factory for remoting::protocol::SessionManager, perhaps it would make more sense under remoting/protocol/? The interface could even be defined in remoting/protocol/session_manager.h to save on a bit of boilerplate, if you wanted. http://codereview.chromium.org/10538091/diff/1/remoting/host/simple_host_proc... File remoting/host/simple_host_process.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/simple_host_proc... remoting/host/simple_host_process.cc:241: session_manager_factory.Pass(), Pass the new object directly here?
http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.cc File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:70: session_manager_factory_(session_manager_factory.release()), On 2012/06/11 22:17:06, Wez wrote: > release() -> Pass() Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:101: network_settings_, context_->url_request_context_getter()).release()); On 2012/06/11 22:17:06, Wez wrote: > release() -> Pass() This is a release()-reset() pair, so I don't think Pass() is right. swap() would be cleaner, but maybe it's too indirect. http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host_... File remoting/host/chromoting_host_unittest.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host_... remoting/host/chromoting_host_unittest.cc:100: session_manager_factory.Pass(), On 2012/06/11 22:17:06, Wez wrote: > Do you need to assign the Mock to a temporary, above? Can't you just pass in the > new object directly here? The mock will need more setting up when the unit tests are re-enabled. It'll be clearer to spread the code out, and it'll be a smaller change if I leave it this way for now. http://codereview.chromium.org/10538091/diff/1/remoting/host/host_mock_objects.h File remoting/host/host_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/host_mock_object... remoting/host/host_mock_objects.h:153: network_settings, url_request_context_getter)); On 2012/06/11 22:17:06, Wez wrote: > Although it seems a bit pointless, I think this belongs in the .cc file... This way is consistent with protocol_mock_objects.h, and I don't think it matters in test code, especially when it's reducing boiler-plate so much. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... File remoting/host/jingle_session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:1: #include "remoting/host/jingle_session_manager_factory.h" On 2012/06/11 22:17:06, Wez wrote: > You're missing a Copyright header. Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:22: // Create port allocator and transport factory. On 2012/06/11 22:17:06, Wez wrote: > nit: Suggest "Create a cricket::PortAllocator that uses Chrome's network stack"? Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:27: bool incoming_only = network_settings.nat_traversal_mode == On 2012/06/11 22:17:06, Wez wrote: > nit: Suggest comment "Create a libjingle TransportFactory implementation, using > the Chrome-based PortAllocator." Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.cc:35: // Create and session manager. On 2012/06/11 22:17:06, Wez wrote: > nit: Suggest "Create a Jingle-protocol SessionManager that uses the libjingle > TransportFactory." Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... File remoting/host/jingle_session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:14: class JingleSessionManagerFactory : public SessionManagerFactory { On 2012/06/11 22:17:06, Wez wrote: > Add a brief comment explaining what this factory makes. Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:17: virtual ~JingleSessionManagerFactory() OVERRIDE; On 2012/06/11 22:17:06, Wez wrote: > You don't need OVERRIDE on dtors. Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:19: virtual scoped_ptr<protocol::SessionManager> MakeSessionManager( On 2012/06/11 22:17:06, Wez wrote: > nit: CreateSessionManager is more in keeping with *Factory method naming > elsewhere in remoting/ Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/jingle_session_m... remoting/host/jingle_session_manager_factory.h:22: url_request_context_getter) OVERRIDE; On 2012/06/11 22:17:06, Wez wrote: > You're missing the disallow-copy-and-assign macro here. Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... remoting/host/plugin/host_script_object.cc:557: // Create the session manager factory. On 2012/06/11 22:17:06, Wez wrote: > nit: Suggest e.g. "Create a SessionManagerFactory that uses Jingle on Chrome's > network stack". Removed code. http://codereview.chromium.org/10538091/diff/1/remoting/host/plugin/host_scri... remoting/host/plugin/host_script_object.cc:565: session_manager_factory.Pass(), On 2012/06/11 22:17:06, Wez wrote: > Do you need to assign to a local & Pass(), or could you just pass the new object > directly? Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/remoting_me2me_h... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/remoting_me2me_h... remoting/host/remoting_me2me_host.cc:389: session_manager_factory.Pass(), network_settings); On 2012/06/11 22:17:06, Wez wrote: > Pass the new object directly here? Done. http://codereview.chromium.org/10538091/diff/1/remoting/host/session_manager_... File remoting/host/session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/session_manager_... remoting/host/session_manager_factory.h:6: #define REMOTING_HOST_SESSION_MANAGER_FACTORY_H_ On 2012/06/11 22:17:06, Wez wrote: > Since this is a factory for remoting::protocol::SessionManager, perhaps it would > make more sense under remoting/protocol/? The interface could even be defined > in remoting/protocol/session_manager.h to save on a bit of boilerplate, if you > wanted. That's where I put it to begin with, but NetworkSettings is in remoting/host, and protocol_mock_objects.h would have had to include it. http://codereview.chromium.org/10538091/diff/1/remoting/host/simple_host_proc... File remoting/host/simple_host_process.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/simple_host_proc... remoting/host/simple_host_process.cc:241: session_manager_factory.Pass(), On 2012/06/11 22:17:06, Wez wrote: > Pass the new object directly here? Done.
http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... remoting/host/chromoting_host.cc:66: scoped_ptr<SessionManagerFactory> session_manager_factory, Instead of passing the factory here can we pass the SessionManager iteself? It's created only once in the Start() method and Start() is always called after the constructor.
On 2012/06/11 23:43:37, sergeyu wrote: > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > File remoting/host/chromoting_host.cc (right): > > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > remoting/host/chromoting_host.cc:66: scoped_ptr<SessionManagerFactory> > session_manager_factory, > Instead of passing the factory here can we pass the SessionManager iteself? It's > created only once in the Start() method and Start() is always called after the > constructor. I don't know the answer to that question. Does it matter if the SessionManager is constructed before the ChromotingHost? Does it matter what thread the SessionManager is constructed on? But I do know that this refactoring doesn't change behaviour, and lets us move towards re-enabling unit tests.
On 2012/06/12 00:10:34, simonmorris wrote: > On 2012/06/11 23:43:37, sergeyu wrote: > > > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > > File remoting/host/chromoting_host.cc (right): > > > > > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > > remoting/host/chromoting_host.cc:66: scoped_ptr<SessionManagerFactory> > > session_manager_factory, > > Instead of passing the factory here can we pass the SessionManager iteself? > It's > > created only once in the Start() method and Start() is always called after the > > constructor. > > I don't know the answer to that question. Does it matter if the SessionManager > is constructed before the ChromotingHost? Does it matter what thread the > SessionManager is constructed on? But I do know that this refactoring doesn't > change behaviour, and lets us move towards re-enabling unit tests. Construction of both ChromotingHost and SessionManager happens on the network thread so there should be no threading issues. Everything should work properly if you just pass SessionManager to the constructor or Start(). More than that: it should be possible to merge Start() method with the constructor - we needed Start() method in the past when it was called on a thread different from the thread on which the object was constructed. We don't need it anymore. Constructing SessionManager outside of Chromoting host will make the constructor much simpler. E.g. the constructor takes signal_strategy and network_settings just to pass them to the SessionManager. If you move SessionManager creation out of ChromotingHost then the we can simplify ChromotingHost constructor.
On 2012/06/12 00:20:54, sergeyu wrote: > On 2012/06/12 00:10:34, simonmorris wrote: > > On 2012/06/11 23:43:37, sergeyu wrote: > > > > > > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > > > File remoting/host/chromoting_host.cc (right): > > > > > > > > > http://codereview.chromium.org/10538091/diff/1016/remoting/host/chromoting_ho... > > > remoting/host/chromoting_host.cc:66: scoped_ptr<SessionManagerFactory> > > > session_manager_factory, > > > Instead of passing the factory here can we pass the SessionManager iteself? > > It's > > > created only once in the Start() method and Start() is always called after > the > > > constructor. > > > > I don't know the answer to that question. Does it matter if the SessionManager > > is constructed before the ChromotingHost? Does it matter what thread the > > SessionManager is constructed on? But I do know that this refactoring doesn't > > change behaviour, and lets us move towards re-enabling unit tests. > > Construction of both ChromotingHost and SessionManager happens on the network > thread so there should be no threading issues. Everything should work properly > if you just pass SessionManager to the constructor or Start(). > More than that: it should be possible to merge Start() method with the > constructor - we needed Start() method in the past when it was called on a > thread different from the thread on which the object was constructed. We don't > need it anymore. > Constructing SessionManager outside of Chromoting host will make the constructor > much simpler. E.g. the constructor takes signal_strategy and network_settings > just to pass them to the SessionManager. If you move SessionManager creation out > of ChromotingHost then the we can simplify ChromotingHost constructor. OK - I'll make a separate CL with the changes you suggest.
ptal I've removed SessionManagerFactory, and stopped passing NetworkSettings to ChromotingHost. The other changes sergeyu@ suggested can be made in a separate CL.
http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.cc File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:101: network_settings_, context_->url_request_context_getter()).release()); On 2012/06/11 23:08:59, simonmorris wrote: > On 2012/06/11 22:17:06, Wez wrote: > > release() -> Pass() > > This is a release()-reset() pair, so I don't think Pass() is right. > > swap() would be cleaner, but maybe it's too indirect. Right - it should be session_manager_ = ___.Pass(); http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host.cc:97: // Start session manager. nit: Suggest "Start the SessionManager, supplying the ChromotingHost as the Listener" http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... File remoting/host/chromoting_host_unittest.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host_unittest.cc:100: session_manager.Pass()); nit: Pass in the new mock object directly, rather than via a local var? http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... File remoting/host/host_mock_objects.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... remoting/host/host_mock_objects.cc:9: #include "remoting/host/network_settings.h" Do you need this include? http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... File remoting/host/host_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... remoting/host/host_mock_objects.h:17: #include "remoting/host/network_settings.h" Do you still need this include? http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... File remoting/host/jingle_session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:17: class JingleSessionManagerFactory { There's really no need for this class now that the Create method is static. http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:19: static scoped_ptr<protocol::SessionManager> CreateSessionManager( This factory function creates a SessionManager based on a host-specific PortAllocator, and has an interface that accepts host-specific NetworkSettings, so it's not a general Jingle-based SessionManager factory as such; it should probably be CreateHostSessionManager to match HostPortAllocator. It might also make calling code sites easier to read if this were implemented as a HostSessionManager class, inheriting from JingleSessionManager and doing the necessary initialization in the ctor. http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... File remoting/protocol/protocol_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... remoting/protocol/protocol_mock_objects.h:200: authenticator.release(), Shouldn't these be get(), so that they get clean-up correctly after the mocked ConnectPtr has been called to check the parameter values?
looks mostly good to me. Just some nits. http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host.cc:98: session_manager_->Init(signal_strategy_, this); It may be better to separate Init() from SetListener(). Then here you could just call SetListener() and the calling code would call Init(). That way it won't be necessary to pass signal_strategy_ to ChromotingHost. http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... File remoting/host/jingle_session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:19: static scoped_ptr<protocol::SessionManager> CreateSessionManager( I think it might be better to move it to JinlgeSessionManager class and call it CreateForHost() or something line that. http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... remoting/host/plugin/host_script_object.cc:561: host_context_.get(), nit: IMHO the code is more readable when if we keep these three arguments on one line. http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:388: desktop_environment_.get(), keep these arguments on one line? http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... File remoting/host/simple_host_process.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... remoting/host/simple_host_process.cc:240: desktop_environment_.get(), here too.
http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.cc File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... remoting/host/chromoting_host.cc:101: network_settings_, context_->url_request_context_getter()).release()); On 2012/06/12 21:20:10, Wez wrote: > On 2012/06/11 23:08:59, simonmorris wrote: > > On 2012/06/11 22:17:06, Wez wrote: > > > release() -> Pass() > > > > This is a release()-reset() pair, so I don't think Pass() is right. > > > > swap() would be cleaner, but maybe it's too indirect. > > Right - it should be session_manager_ = ___.Pass(); Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host.cc:97: // Start session manager. On 2012/06/12 21:20:10, Wez wrote: > nit: Suggest "Start the SessionManager, supplying the ChromotingHost as the > Listener" Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host.cc:98: session_manager_->Init(signal_strategy_, this); On 2012/06/13 01:57:19, sergeyu wrote: > It may be better to separate Init() from SetListener(). Then here you could just > call SetListener() and the calling code would call Init(). That way it won't be > necessary to pass signal_strategy_ to ChromotingHost. Sounds good, but that can be done in a separate CL. http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... File remoting/host/chromoting_host_unittest.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... remoting/host/chromoting_host_unittest.cc:100: session_manager.Pass()); On 2012/06/12 21:20:10, Wez wrote: > nit: Pass in the new mock object directly, rather than via a local var? In a subsequent CL, the mock object will have expectations set, so it'll need to be a local variable. The change will be easier to read if I keep it this way for now. http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... File remoting/host/host_mock_objects.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... remoting/host/host_mock_objects.cc:9: #include "remoting/host/network_settings.h" On 2012/06/12 21:20:10, Wez wrote: > Do you need this include? Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... File remoting/host/host_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... remoting/host/host_mock_objects.h:17: #include "remoting/host/network_settings.h" On 2012/06/12 21:20:10, Wez wrote: > Do you still need this include? Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... File remoting/host/jingle_session_manager_factory.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:17: class JingleSessionManagerFactory { On 2012/06/12 21:20:10, Wez wrote: > There's really no need for this class now that the Create method is static. Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:19: static scoped_ptr<protocol::SessionManager> CreateSessionManager( On 2012/06/12 21:20:10, Wez wrote: > This factory function creates a SessionManager based on a host-specific > PortAllocator, and has an interface that accepts host-specific NetworkSettings, > so it's not a general Jingle-based SessionManager factory as such; it should > probably be CreateHostSessionManager to match HostPortAllocator. > Done. > It might also make calling code sites easier to read if this were implemented as > a HostSessionManager class, inheriting from JingleSessionManager and doing the > necessary initialization in the ctor. A separate class, that inherits implementation, is too heavyweight if the only benefit is replacing a Create* function with a constructor. http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... remoting/host/jingle_session_manager_factory.h:19: static scoped_ptr<protocol::SessionManager> CreateSessionManager( On 2012/06/13 01:57:19, sergeyu wrote: > I think it might be better to move it to JinlgeSessionManager class and call it > CreateForHost() or something line that. JingleSessionManager is in remoting/protocol, so shouldn't depend on remoting/host. http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... remoting/host/plugin/host_script_object.cc:561: host_context_.get(), On 2012/06/13 01:57:19, sergeyu wrote: > nit: IMHO the code is more readable when if we keep these three arguments on one > line. Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:388: desktop_environment_.get(), On 2012/06/13 01:57:19, sergeyu wrote: > keep these arguments on one line? Done. http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... File remoting/host/simple_host_process.cc (right): http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... remoting/host/simple_host_process.cc:240: desktop_environment_.get(), On 2012/06/13 01:57:19, sergeyu wrote: > here too. Done. http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... File remoting/protocol/protocol_mock_objects.h (right): http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... remoting/protocol/protocol_mock_objects.h:200: authenticator.release(), On 2012/06/12 21:20:10, Wez wrote: > Shouldn't these be get(), so that they get clean-up correctly after the mocked > ConnectPtr has been called to check the parameter values? Done.
On 2012/06/13 16:26:33, simonmorris wrote: > http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.cc > File remoting/host/chromoting_host.cc (right): > > http://codereview.chromium.org/10538091/diff/1/remoting/host/chromoting_host.... > remoting/host/chromoting_host.cc:101: network_settings_, > context_->url_request_context_getter()).release()); > On 2012/06/12 21:20:10, Wez wrote: > > On 2012/06/11 23:08:59, simonmorris wrote: > > > On 2012/06/11 22:17:06, Wez wrote: > > > > release() -> Pass() > > > > > > This is a release()-reset() pair, so I don't think Pass() is right. > > > > > > swap() would be cleaner, but maybe it's too indirect. > > > > Right - it should be session_manager_ = ___.Pass(); > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... > File remoting/host/chromoting_host.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... > remoting/host/chromoting_host.cc:97: // Start session manager. > On 2012/06/12 21:20:10, Wez wrote: > > nit: Suggest "Start the SessionManager, supplying the ChromotingHost as the > > Listener" > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... > remoting/host/chromoting_host.cc:98: session_manager_->Init(signal_strategy_, > this); > On 2012/06/13 01:57:19, sergeyu wrote: > > It may be better to separate Init() from SetListener(). Then here you could > just > > call SetListener() and the calling code would call Init(). That way it won't > be > > necessary to pass signal_strategy_ to ChromotingHost. > > Sounds good, but that can be done in a separate CL. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... > File remoting/host/chromoting_host_unittest.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/chromoting_h... > remoting/host/chromoting_host_unittest.cc:100: session_manager.Pass()); > On 2012/06/12 21:20:10, Wez wrote: > > nit: Pass in the new mock object directly, rather than via a local var? > > In a subsequent CL, the mock object will have expectations set, so it'll need to > be a local variable. The change will be easier to read if I keep it this way for > now. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... > File remoting/host/host_mock_objects.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... > remoting/host/host_mock_objects.cc:9: #include > "remoting/host/network_settings.h" > On 2012/06/12 21:20:10, Wez wrote: > > Do you need this include? > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... > File remoting/host/host_mock_objects.h (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/host_mock_ob... > remoting/host/host_mock_objects.h:17: #include > "remoting/host/network_settings.h" > On 2012/06/12 21:20:10, Wez wrote: > > Do you still need this include? > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... > File remoting/host/jingle_session_manager_factory.h (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... > remoting/host/jingle_session_manager_factory.h:17: class > JingleSessionManagerFactory { > On 2012/06/12 21:20:10, Wez wrote: > > There's really no need for this class now that the Create method is static. > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... > remoting/host/jingle_session_manager_factory.h:19: static > scoped_ptr<protocol::SessionManager> CreateSessionManager( > On 2012/06/12 21:20:10, Wez wrote: > > This factory function creates a SessionManager based on a host-specific > > PortAllocator, and has an interface that accepts host-specific > NetworkSettings, > > so it's not a general Jingle-based SessionManager factory as such; it should > > probably be CreateHostSessionManager to match HostPortAllocator. > > > > Done. > > > It might also make calling code sites easier to read if this were implemented > as > > a HostSessionManager class, inheriting from JingleSessionManager and doing the > > necessary initialization in the ctor. > > A separate class, that inherits implementation, is too heavyweight if the only > benefit is replacing a Create* function with a constructor. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/jingle_sessi... > remoting/host/jingle_session_manager_factory.h:19: static > scoped_ptr<protocol::SessionManager> CreateSessionManager( > On 2012/06/13 01:57:19, sergeyu wrote: > > I think it might be better to move it to JinlgeSessionManager class and call > it > > CreateForHost() or something line that. > > JingleSessionManager is in remoting/protocol, so shouldn't depend on > remoting/host. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... > File remoting/host/plugin/host_script_object.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/plugin/host_... > remoting/host/plugin/host_script_object.cc:561: host_context_.get(), > On 2012/06/13 01:57:19, sergeyu wrote: > > nit: IMHO the code is more readable when if we keep these three arguments on > one > > line. > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... > File remoting/host/remoting_me2me_host.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/remoting_me2... > remoting/host/remoting_me2me_host.cc:388: desktop_environment_.get(), > On 2012/06/13 01:57:19, sergeyu wrote: > > keep these arguments on one line? > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... > File remoting/host/simple_host_process.cc (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/host/simple_host_... > remoting/host/simple_host_process.cc:240: desktop_environment_.get(), > On 2012/06/13 01:57:19, sergeyu wrote: > > here too. > > Done. > > http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... > File remoting/protocol/protocol_mock_objects.h (right): > > http://codereview.chromium.org/10538091/diff/14001/remoting/protocol/protocol... > remoting/protocol/protocol_mock_objects.h:200: authenticator.release(), > On 2012/06/12 21:20:10, Wez wrote: > > Shouldn't these be get(), so that they get clean-up correctly after the mocked > > ConnectPtr has been called to check the parameter values? > > Done. ping
LGTM, although having read back the comments in session_manager_factory.cc I've tried to suggest some more descriptive rewrites... up to you whether to use them! http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... File remoting/host/session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:19: // Create a cricket::PortAllocator that uses Chrome's network stack. nit: Suggest "Use Chrome's network stack to allocate ports for peer-to-peer channels." http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:25: NetworkSettings::NAT_TRAVERSAL_DISABLED; nit: Add a comment e.g. "Use libjingle for negotiation of peer-to-peer channels over HostPortAllocator allocated ports." http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:33: // TransportFactory. nit: Suggest "Use the Jingle protocol for channel-negotiation signalling between peer TransportFactories." (Sorry; reading this back I realise the comment I suggested previously was not very clear ): http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:36: scoped_ptr<protocol::JingleSessionManager> session_manager( nit: Add a blank line after the fetch_stun_relay_info definition, or remove the one after incoming_only, for consistency.
ping sergeyu@ http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... File remoting/host/session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:19: // Create a cricket::PortAllocator that uses Chrome's network stack. On 2012/06/14 01:00:58, Wez wrote: > nit: Suggest "Use Chrome's network stack to allocate ports for peer-to-peer > channels." Done. http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:25: NetworkSettings::NAT_TRAVERSAL_DISABLED; On 2012/06/14 01:00:58, Wez wrote: > nit: Add a comment e.g. "Use libjingle for negotiation of peer-to-peer channels > over HostPortAllocator allocated ports." Done. http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:33: // TransportFactory. On 2012/06/14 01:00:58, Wez wrote: > nit: Suggest "Use the Jingle protocol for channel-negotiation signalling between > peer TransportFactories." > > (Sorry; reading this back I realise the comment I suggested previously was not > very clear ): Done. http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... remoting/host/session_manager_factory.cc:36: scoped_ptr<protocol::JingleSessionManager> session_manager( On 2012/06/14 01:00:58, Wez wrote: > nit: Add a blank line after the fetch_stun_relay_info definition, or remove the > one after incoming_only, for consistency. Done.
looking now On 2012/06/14 01:24:02, simonmorris wrote: > ping sergeyu@ > > http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... > File remoting/host/session_manager_factory.cc (right): > > http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... > remoting/host/session_manager_factory.cc:19: // Create a cricket::PortAllocator > that uses Chrome's network stack. > On 2012/06/14 01:00:58, Wez wrote: > > nit: Suggest "Use Chrome's network stack to allocate ports for peer-to-peer > > channels." > > Done. > > http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... > remoting/host/session_manager_factory.cc:25: > NetworkSettings::NAT_TRAVERSAL_DISABLED; > On 2012/06/14 01:00:58, Wez wrote: > > nit: Add a comment e.g. "Use libjingle for negotiation of peer-to-peer > channels > > over HostPortAllocator allocated ports." > > Done. > > http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... > remoting/host/session_manager_factory.cc:33: // TransportFactory. > On 2012/06/14 01:00:58, Wez wrote: > > nit: Suggest "Use the Jingle protocol for channel-negotiation signalling > between > > peer TransportFactories." > > > > (Sorry; reading this back I realise the comment I suggested previously was not > > very clear ): > > Done. > > http://codereview.chromium.org/10538091/diff/10020/remoting/host/session_mana... > remoting/host/session_manager_factory.cc:36: > scoped_ptr<protocol::JingleSessionManager> session_manager( > On 2012/06/14 01:00:58, Wez wrote: > > nit: Add a blank line after the fetch_stun_relay_info definition, or remove > the > > one after incoming_only, for consistency. > > Done.
lgtm http://codereview.chromium.org/10538091/diff/5019/remoting/host/session_manag... File remoting/host/session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/5019/remoting/host/session_manag... remoting/host/session_manager_factory.cc:25: // HostPortAllocator allocated ports. nit: this comment is confusing here. Move it below to where the transport factory is created?
fyi http://codereview.chromium.org/10538091/diff/5019/remoting/host/session_manag... File remoting/host/session_manager_factory.cc (right): http://codereview.chromium.org/10538091/diff/5019/remoting/host/session_manag... remoting/host/session_manager_factory.cc:25: // HostPortAllocator allocated ports. On 2012/06/14 18:24:30, sergeyu wrote: > nit: this comment is confusing here. Move it below to where the transport > factory is created? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10538091/21001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10538091/18008
Try job failure for 10538091-18008 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10538091/18008
Change committed as 142473 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
