Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6623048:
Replace libjingle's HttpPortAllocatorSession with Pepper's http client (Closed)
|
Created:
9 years, 9 months ago by Alpha Left Google Modified:
9 years, 6 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace libjingle's HttpPortAllocatorSession with Pepper's http client
HttpPortAllocatorSession uses libjingle's http client to establish relay
connection. This will not work in the sanboxed case since it access OS
level of network directly.
This patch replaces the http client with URL loader in pepper. This goes
through the standard URL fetching system for chrome to get around the
sandbox problem.
BUG=51198
TEST=Force libjingle to use relay and try it with this code.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77218
Patch Set 1 #Patch Set 2 : thread switching #
Total comments: 20
Patch Set 3 : done #Patch Set 4 : mergedf #Patch Set 5 : changes done #Messages
Total messages: 7 (0 generated)
http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_port_allocator_session.cc (right): http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:31: class SessionFactory : public remoting::PortAllocatorSessionFactory { TODO to move this to a separate file? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:64: void Trim(std::string& str) { Can we use base::TrimString() instead? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:86: end_of_line = string.find_first_of("\r\n", start_of_line); Could it be that the result we receive from the server contains Windows style line ends? If yes, does this method parses them properly? If not, why should we care about \r? Also, Can we use base::Tokenize() to split the result lines? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:91: size_t equals = string.find('=', start_of_line); equals_pos? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:111: class PepperURLFetcher { TODO to move this to a separate file? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:248: NewRunnableMethod(this, &PepperCreateSessionTask::Dummy)); This doesn't guarantee that the object will be destroyed on that thread, does it? Maybe replace Dummy with a method that explicitly destroys PepperURLFetcher on that thread? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:260: base::AutoLock auto_lock(lock_); Can we use MessageLoopProxy here? You would not need the lock? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:430: DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::PepperPortAllocatorSession); I don't see why we would need this. Remove it? http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_port_allocator_session.h (right): http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.h:36: virtual void SendSessionRequest(const std::string& host, int port); Add comments that these are overrides. Maybe add OVERRIDE. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.h:42: static PortAllocatorSessionFactory* CreateFactory( It doesn't look right, that the object creates factory for itself. I think this should be method of the factory itself.
http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_port_allocator_session.cc (right): http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:31: class SessionFactory : public remoting::PortAllocatorSessionFactory { On 2011/03/07 12:58:45, sergeyu wrote: > TODO to move this to a separate file? Done. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:64: void Trim(std::string& str) { On 2011/03/07 12:58:45, sergeyu wrote: > Can we use base::TrimString() instead? Done. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:86: end_of_line = string.find_first_of("\r\n", start_of_line); On 2011/03/07 12:58:45, sergeyu wrote: > Could it be that the result we receive from the server contains Windows style > line ends? If yes, does this method parses them properly? If not, why should we > care about \r? > > Also, Can we use base::Tokenize() to split the result lines? HTTP use \r\n as line breaks and this is from libjingle. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:91: size_t equals = string.find('=', start_of_line); On 2011/03/07 12:58:45, sergeyu wrote: > equals_pos? I'm reluctant to change this at the moment since this is straight from libjingle. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:98: Trim(key); I did change this to base::TrimString(). http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:111: class PepperURLFetcher { On 2011/03/07 12:58:45, sergeyu wrote: > TODO to move this to a separate file? Done. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:248: NewRunnableMethod(this, &PepperCreateSessionTask::Dummy)); On 2011/03/07 12:58:45, sergeyu wrote: > This doesn't guarantee that the object will be destroyed on that thread, does > it? Maybe replace Dummy with a method that explicitly destroys PepperURLFetcher > on that thread? sounds good. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.cc:260: base::AutoLock auto_lock(lock_); On 2011/03/07 12:58:45, sergeyu wrote: > Can we use MessageLoopProxy here? You would not need the lock? I believe we have to use message loop proxy everywhere in order to take advantage of MessageLoopProxy. Right now using a lock does the same thing. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_port_allocator_session.h (right): http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.h:36: virtual void SendSessionRequest(const std::string& host, int port); On 2011/03/07 12:58:45, sergeyu wrote: > Add comments that these are overrides. Maybe add OVERRIDE. Done. http://codereview.chromium.org/6623048/diff/3001/remoting/client/plugin/peppe... remoting/client/plugin/pepper_port_allocator_session.h:42: static PortAllocatorSessionFactory* CreateFactory( On 2011/03/07 12:58:45, sergeyu wrote: > It doesn't look right, that the object creates factory for itself. I think this > should be method of the factory itself. I'll make this a method in the namespace but I don't think there's much difference being a global method or a class static method.
LGTM
Sorry, I've only just had a chance to look through this properly. I don't understand why we need PortAllocatorSessionFactory - the code it gets supplied to is the HttpPortAllocator glue, so why not hide the distinction behind HttpPortAllocator, rather than creating another interface through which do so?
This is more about code dependency and structure. You can pass a boolean into HttpPortAllocator to tell it to use JingleHttpPortAllocator, but that breaks dependency. In terms of code structure it shouldn't know about which implementation it uses for port allocator session.
Sorry, I'd missed the fact that remoting::HttpPortAllocator derives from cricket::HttpPortAllocator, so I wondered why the two implementations had what seemed a false dependency. I'd recommend changing the "new" in remoting::HttpPortAllocator::CreateSession to call the base-class version rather than creating the HttpPortAllocatorSession itself. On 23 March 2011 16:45, <hclam@chromium.org> wrote: > This is more about code dependency and structure. > > You can pass a boolean into HttpPortAllocator to tell it to use > JingleHttpPortAllocator, but that breaks dependency. In terms of code > structure > it shouldn't know about which implementation it uses for port allocator > session. > > > > http://codereview.chromium.org/6623048/ > |
