Add static Create method to LibjingleTransportFactory

Add static Create method to LibjingleTransportFactory

This allows the class's easy use in future client implementations.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207942

[solb, 2013-06-20 01:38:37]

I'm using the session_manager_factory header in my code and wanted it to expose
access to its TransportFactory, so I figured I would move it into protocol/
while I was at it.

All our targets still build and every enabled unit test passes.

[Sergey Ulanov, 2013-06-20 19:37:53]

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
File remoting/protocol/native_port_allocator.cc (right):

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
remoting/protocol/native_port_allocator.cc:24: class NativePortAllocatorSession
I think this code belongs to jingle_glue layer, and it's better to call it
ChromiumPortAllocator for consistency with ChromiumSocketFactory. And then
NetworkSettings should also be in jingle_glue.

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
remoting/protocol/native_port_allocator.cc:97: int port) {
nit: indentation

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
File remoting/protocol/session_manager_factory.cc (right):

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
remoting/protocol/session_manager_factory.cc:17:
scoped_ptr<protocol::TransportFactory> CreateTransportFactory(
This function doesn't belong to this file. It would be better to add a static
Create() method in LibjingleTransportFactory.

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
remoting/protocol/session_manager_factory.cc:38:
scoped_ptr<protocol::SessionManager> CreateNativeSessionManager(
This function doesn't do much, so I don't think we need it at all.
I suggest that you keep this file in the host directory, keep this function as
CreateHostSessionManager(), but move LibjingleTransportFactory creation to a
static LibjingleTransportFactory::Create() as I suggested above. For the client
side you don't really need to worry about nat traversal (it's always enabled),
so session manager creation becomes simpler:

scoped_ptr<protocol::SessionManager> session_manager(
    new protocol::JingleSessionManager(
        CreateTransportFactory(network_settings, url_request_context_getter),
        true);

https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
remoting/protocol/session_manager_factory.cc:44: url_request_context_getter));
this argument should be aligned with the first argument on the previous line.

[solb, 2013-06-20 23:58:33]

On 2013/06/20 19:37:53, Sergey Ulanov wrote:
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
> File remoting/protocol/native_port_allocator.cc (right):
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
> remoting/protocol/native_port_allocator.cc:24: class
NativePortAllocatorSession
> I think this code belongs to jingle_glue layer, and it's better to call it
> ChromiumPortAllocator for consistency with ChromiumSocketFactory. And then
> NetworkSettings should also be in jingle_glue.
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/native_port...
> remoting/protocol/native_port_allocator.cc:97: int port) {
> nit: indentation
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
> File remoting/protocol/session_manager_factory.cc (right):
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
> remoting/protocol/session_manager_factory.cc:17:
> scoped_ptr<protocol::TransportFactory> CreateTransportFactory(
> This function doesn't belong to this file. It would be better to add a static
> Create() method in LibjingleTransportFactory.
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
> remoting/protocol/session_manager_factory.cc:38:
> scoped_ptr<protocol::SessionManager> CreateNativeSessionManager(
> This function doesn't do much, so I don't think we need it at all.
> I suggest that you keep this file in the host directory, keep this function as
> CreateHostSessionManager(), but move LibjingleTransportFactory creation to a
> static LibjingleTransportFactory::Create() as I suggested above. For the
client
> side you don't really need to worry about nat traversal (it's always enabled),
> so session manager creation becomes simpler:
> 
> scoped_ptr<protocol::SessionManager> session_manager(
>     new protocol::JingleSessionManager(
>         CreateTransportFactory(network_settings, url_request_context_getter),
>         true);
> 
>
https://codereview.chromium.org/17101034/diff/1/remoting/protocol/session_man...
> remoting/protocol/session_manager_factory.cc:44: url_request_context_getter));
> this argument should be aligned with the first argument on the previous line.

Sergey,
I restructured this according to your comments; are your concerns addressed?

[Sergey Ulanov, 2013-06-21 00:48:13]

https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
File remoting/host/session_manager_factory.cc (right):

https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
remoting/host/session_manager_factory.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
It's strange that rietveld shows this file as deleted (D in the list of files),
but here it looks like you are just changing it. CQ will probably be able to
land it anyway, so this is just FYI if there are any problems when landing it.

https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
remoting/host/session_manager_factory.cc:21: url_request_context_getter));
incorrect indentation.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/17101034/diff/6001/remoting/jingle_glue/chrom...
File remoting/jingle_glue/chromium_port_allocator.cc (right):

https://codereview.chromium.org/17101034/diff/6001/remoting/jingle_glue/chrom...
remoting/jingle_glue/chromium_port_allocator.cc:95: const std::string& host, int
port) {
nit: If all arguments in function definition don't fit on one line with the
function name then you should put one argument per line.

It should be formatted as follows:
  void ChromiumPortAllocatorSession::SendSessionRequest(
      const std::string& host,
      int port)

https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
remoting/protocol/libjingle_transport_factory.cc:326: {
this bracket goes to the previous line.

https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
File remoting/protocol/libjingle_transport_factory.h (right):

https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
remoting/protocol/libjingle_transport_factory.h:32: static
scoped_ptr<LibjingleTransportFactory> Create(
Add comments saying that this method creates LibjingleTransportFactory with
ChromiumSocketFactory. Also mention that it must be called on an IO thread.

[solb, 2013-06-21 01:05:45]

On 2013/06/21 00:48:13, Sergey Ulanov wrote:
>
https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
> File remoting/host/session_manager_factory.cc (right):
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
> remoting/host/session_manager_factory.cc:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> It's strange that rietveld shows this file as deleted (D in the list of
files),
> but here it looks like you are just changing it. CQ will probably be able to
> land it anyway, so this is just FYI if there are any problems when landing it.
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/host/session_mana...
> remoting/host/session_manager_factory.cc:21: url_request_context_getter));
> incorrect indentation.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/jingle_glue/chrom...
> File remoting/jingle_glue/chromium_port_allocator.cc (right):
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/jingle_glue/chrom...
> remoting/jingle_glue/chromium_port_allocator.cc:95: const std::string& host,
int
> port) {
> nit: If all arguments in function definition don't fit on one line with the
> function name then you should put one argument per line.
> 
> It should be formatted as follows:
>   void ChromiumPortAllocatorSession::SendSessionRequest(
>       const std::string& host,
>       int port)
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
> File remoting/protocol/libjingle_transport_factory.cc (right):
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
> remoting/protocol/libjingle_transport_factory.cc:326: {
> this bracket goes to the previous line.
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
> File remoting/protocol/libjingle_transport_factory.h (right):
> 
>
https://codereview.chromium.org/17101034/diff/6001/remoting/protocol/libjingl...
> remoting/protocol/libjingle_transport_factory.h:32: static
> scoped_ptr<LibjingleTransportFactory> Create(
> Add comments saying that this method creates LibjingleTransportFactory with
> ChromiumSocketFactory. Also mention that it must be called on an IO thread.

Done.  Sorry about the style violations!

[Sergey Ulanov, 2013-06-21 01:21:57]

LGTM when my comments are addressed.

https://codereview.chromium.org/17101034/diff/17001/remoting/jingle_glue/chro...
File remoting/jingle_glue/chromium_port_allocator.h (right):

https://codereview.chromium.org/17101034/diff/17001/remoting/jingle_glue/chro...
remoting/jingle_glue/chromium_port_allocator.h:23: // used by the remoting host.
The main difference from
This comment needs to be updated as this class is not host specific anymore.
e.g.:

// An implementation of cricket::PortAllocator for libjingle that 
// uses Chromium's network stack and configures itself according to
// the specified NetworkSettings.

https://codereview.chromium.org/17101034/diff/17001/remoting/protocol/libjing...
File remoting/protocol/libjingle_transport_factory.h (right):

https://codereview.chromium.org/17101034/diff/17001/remoting/protocol/libjing...
remoting/protocol/libjingle_transport_factory.h:32: // Creates an instance of
the class using ChromiumSocketFactory.
nit: actually it should be ChromiumPortAllocator.

https://codereview.chromium.org/17101034/diff/17001/remoting/protocol/libjing...
remoting/protocol/libjingle_transport_factory.h:33: // Performs network
operations, so must be called from IO thread.
nit: remove "Performs network operations" - it's not correct (this function just
allocates object that later may use network).

[commit-bot: I haz the power, 2013-06-21 01:52:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/17101034/26001

[Sergey Ulanov, 2013-06-21 02:05:28]

https://codereview.chromium.org/17101034/diff/26001/remoting/jingle_glue/chro...
File remoting/jingle_glue/chromium_port_allocator.h (right):

https://codereview.chromium.org/17101034/diff/26001/remoting/jingle_glue/chro...
remoting/jingle_glue/chromium_port_allocator.h:24: // when creating relay
sessions. It also configures itself according
I think you want to remove this line and 'to' from the next line.

[commit-bot: I haz the power, 2013-06-21 02:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/17101034/17003

[commit-bot: I haz the power, 2013-06-21 03:10:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 16:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/17101034/40001

[commit-bot: I haz the power, 2013-06-21 17:12:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 17:14:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/17101034/49002

[commit-bot: I haz the power, 2013-06-21 22:42:58]

Change committed as 207942