Websocket to TCP proxy running in a separate thread (only on ChromeOS).

Websocket to TCP proxy running in a separate thread (only on ChromeOS).

With this webproxy: authorized extensions can connect to ws://127.0.0.1:10101/tcpproxy,
pass authorization_token:hostname:port: in first frame,
then webproxy establishes TCP connection to hostname:port and forwards any subsequent
communication.
Subsequent communication between extension and webproxy is base64-encoded.

TODO(dilmah): remove this temporary solution, get rid of separate thread and listening socket,
instead provide the same functionality via hooks into websocket layer.

BUG=chromium-os:9667
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84795

[Denis Lagno, 2011-04-06 14:41:32]

please take a look.

This is continuation of http://codereview.chromium.org/5484001/

[zel, 2011-04-06 14:47:37]

first, please move code from chrome/browser/net to
chrome/browser/chromeos/net

On Wed, Apr 6, 2011 at 7:41 AM, <dilmah@chromium.org> wrote:

> Reviewers: zel, jar, Avi,
>
> Message:
> please take a look.
>
> This is continuation of http://codereview.chromium.org/5484001/
>
> Description:
> Websocket to TCP proxy running in a separate thread (only on ChromeOS).
>
> With this webproxy: authorized extensions can connect to
> ws://127.0.0.1:10101/webproxy,
> pass hostname:port:authorization_token in first frame,
> then webproxy establishes TCP connection to hostname:port and forwards any
> subsequent
> communication.
> Subsequent communication between extension and webproxy is base64-encoded.
>
> TODO(dilmah): remove this temporary solution, get rid of separate thread
> and
> listening socket,
> instead provide the same functionality via hooks into websocket layer.
>
> BUG=chromium-os:9667
> TEST=Manual
>
>
> Please review this at http://codereview.chromium.org/6801008/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/browser_main.cc
>  M chrome/browser/browser_process.h
>  M chrome/browser/browser_process_impl.h
>  M chrome/browser/browser_process_impl.cc
>  A chrome/browser/chromeos/webproxy_task.h
>  A chrome/browser/chromeos/webproxy_task.cc
>  A chrome/browser/net/webproxy/conn.h
>  A chrome/browser/net/webproxy/conn.cc
>  A chrome/browser/net/webproxy/serv.h
>  A chrome/browser/net/webproxy/serv.cc
>  M chrome/chrome_browser.gypi
>  M chrome/test/testing_browser_process.h
>  M chrome/test/testing_browser_process.cc
>  M content/browser/browser_thread.h
>  M content/browser/browser_thread.cc
>  M third_party/libevent/chromium.patch
>  M third_party/libevent/evdns.h
>
>
>

[Denis Lagno, 2011-04-06 15:24:53]

On 2011/04/06 14:47:37, zel wrote:
> first, please move code from chrome/browser/net to
> chrome/browser/chromeos/net

moved

[zel, 2011-04-07 16:54:11]

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1478: // We delay launching websocket to TCP
proxy because we do not want to
can we make this launch when API is being used for the first time instead? same
goes for process->webproxy_thread(). Most of the users won't really have this
running anyway.

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/webproxy/conn.h (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/webproxy/conn.h:17: class Conn {
comments please - what does this class do?

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/webproxy/serv.h (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/webproxy/serv.h:21: class Serv {
comments please - what does this class do?

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/webp...
File chrome/browser/chromeos/webproxy_task.cc (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/webp...
chrome/browser/chromeos/webproxy_task.cc:21:
allowed_origins.push_back("chrome-extension://XXXXXXXXXXXXXXXXXXXXXX");
let's replace one of these with haiffjcadagjlijoggckpgfnoeiflnem and define the
origins as constants

http://codereview.chromium.org/6801008/diff/1019/third_party/libevent/evdns.h
File third_party/libevent/evdns.h (right):

http://codereview.chromium.org/6801008/diff/1019/third_party/libevent/evdns.h...
third_party/libevent/evdns.h:168: #include "evutil.h"
why this change?

[Avi (use Gerrit), 2011-04-07 17:01:05]

content/ change LGTM

[Denis Lagno, 2011-04-07 17:48:22]

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1478: // We delay launching websocket to TCP
proxy because we do not want to
On 2011/04/07 16:54:12, zel wrote:
> can we make this launch when API is being used for the first time instead?
same
> goes for process->webproxy_thread(). Most of the users won't really have this
> running anyway.

it is possible but in this case I need to either merge this CL with private
JSAPI CL into single CL, or just not to launch proxy in this CL at all.

http://codereview.chromium.org/6801008/diff/1019/third_party/libevent/evdns.h
File third_party/libevent/evdns.h (right):

http://codereview.chromium.org/6801008/diff/1019/third_party/libevent/evdns.h...
third_party/libevent/evdns.h:168: #include "evutil.h"
On 2011/04/07 16:54:12, zel wrote:
> why this change?

because it tries to include <evutil.h> from libevent installed system-wide. 
Instead we want to include from third_party/libevent/
So if libevent is not installed system-wide then it would just fail to compile.
If libevent is installed system-wide then we risk to include wrong version of
libevent.

Similar changes are already applied to other third_party/libevent files.  I need
this change just because nobody in chromium tree  used evdns.h yet.

[Denis Lagno, 2011-04-11 23:21:26]

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1478: // We delay launching websocket to TCP
proxy because we do not want to
On 2011/04/07 16:54:12, zel wrote:
> can we make this launch when API is being used for the first time instead?
same
> goes for process->webproxy_thread(). Most of the users won't really have this
> running anyway.

launching webproxy is on demand now.
Idle webproxy thread is still started on startup.

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/webproxy/conn.h (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/webproxy/conn.h:17: class Conn {
On 2011/04/07 16:54:12, zel wrote:
> comments please - what does this class do?

Done.

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/webproxy/serv.h (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/webproxy/serv.h:21: class Serv {
On 2011/04/07 16:54:12, zel wrote:
> comments please - what does this class do?

Done.

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/webp...
File chrome/browser/chromeos/webproxy_task.cc (right):

http://codereview.chromium.org/6801008/diff/1019/chrome/browser/chromeos/webp...
chrome/browser/chromeos/webproxy_task.cc:21:
allowed_origins.push_back("chrome-extension://XXXXXXXXXXXXXXXXXXXXXX");
On 2011/04/07 16:54:12, zel wrote:
> let's replace one of these with haiffjcadagjlijoggckpgfnoeiflnem and define
the
> origins as constants

Done.

[Denis Lagno, 2011-05-06 11:00:07]

how about proceeding with this CL?

[Denis Lagno, 2011-05-10 11:17:40]

existing comments are satisfied.  Waiting either LGTM or additional comments.

[zel, 2011-05-10 16:23:32]

On 2011/05/10 11:17:40, Denis Lagno wrote:
> existing comments are satisfied.  Waiting either LGTM or additional comments.

LGTM

[Evan Martin, 2011-06-23 23:19:09]

On 2011/05/10 16:23:32, zel wrote:
> On 2011/05/10 11:17:40, Denis Lagno wrote:
> > existing comments are satisfied.  Waiting either LGTM or additional
comments.
> 
> LGTM

By the way, can you run new code by someone with readability before checking it
in?  A reviewer should prevent you from checking in code that has no comments on
the class.

[Evan Martin, 2011-06-23 23:20:39]

On 2011/06/23 23:19:09, Evan Martin wrote:
> On 2011/05/10 16:23:32, zel wrote:
> > On 2011/05/10 11:17:40, Denis Lagno wrote:
> > > existing comments are satisfied.  Waiting either LGTM or additional
> comments.
> > 
> > LGTM
> 
> By the way, can you run new code by someone with readability before checking
it
> in?  A reviewer should prevent you from checking in code that has no comments
on
> the class.

(BTW, the code stylistically looks perfect to me -- it's just kinda hard to
figure out what it does. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Comments )