reland: Launch pywebsocket via net::TestServer

Launch pywebsocket via net::TestServer

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

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

[Takashi Toyoshima, 2012-08-27 13:26:44]

Hi bashi.
Could you review this change ahead of proper owner's review.
Because I need a reviewer who is familiar with pywebsocket and chromium
codebase.

[bashi, 2012-08-27 13:57:50]

LGTM.

Looks like you forgot to include base_test_server.h to the CL.

https://chromiumcodereview.appspot.com/10879029/diff/4001/net/tools/testserve...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/10879029/diff/4001/net/tools/testserve...
net/tools/testserver/testserver.py:2155: server_data['port'] =
server._sockets[0][0].getsockname()[1]
I'm not sure why you refer server._sockets[0][0].getsockname()[1] here. Can
|server._sockets[0][0].getsockname()[1]| be different from |server.server_port|?
If so, L2154 would be wrong.

[Takashi Toyoshima, 2012-08-28 05:14:00]

Thanks bashi.
I already landed a change on base_test_server.h to add WebSocket related types.

https://chromiumcodereview.appspot.com/10879029/diff/4001/net/tools/testserve...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/10879029/diff/4001/net/tools/testserve...
net/tools/testserver/testserver.py:2155: server_data['port'] =
server._sockets[0][0].getsockname()[1]
This is for the case where server.server_port is 0.
There, the operating system assigns a random free port instead of 0, then we get
the actual port number via this line.

Anyway, I should log the port number after getting right one.

[Takashi Toyoshima, 2012-08-28 05:29:01]

Ah, sorry set 4 is not enough.
And I'll handle server_port modification in pywebsocket side.

[Takashi Toyoshima, 2012-08-28 06:37:01]

set 5 works with https://codereview.appspot.com/6491043/.
I'll request owners review to others after merging pywebsocket side change.

[bashi, 2012-08-28 06:47:13]

On 2012/08/28 06:37:01, toyoshim wrote:
> set 5 works with https://codereview.appspot.com/6491043/.
> I'll request owners review to others after merging pywebsocket side change.

LGTM.

[Takashi Toyoshima, 2012-08-30 02:11:47]

Hi, Ryan.
Could you review this change?

This change make it possible to launch pywebsocket via net::TestServer as a
non-secure WebSocket server. I'll change some test to use net::TestServer later,
then support secure mode.

This change requires following pywebsocket roll in CQ.
https://chromiumcodereview.appspot.com/10901004/

[Ryan Sleevi, 2012-08-30 02:54:14]

Note: You probably want to find someone familiar with Chromium Python style to
double check (if bashi didn't already), but the other net/ changes look good.

Two questions below before the LG

https://chromiumcodereview.appspot.com/10879029/diff/12002/net/tools/testserv...
File net/tools/testserver/run_testserver.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/12002/net/tools/testserv...
net/tools/testserver/run_testserver.cc:122: if (server_type ==
net::TestServer::TYPE_HTTP)
Do you not need to also have a mapping of TYPE_WS -> TYPE_WSS?

https://chromiumcodereview.appspot.com/10879029/diff/12002/net/tools/testserv...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/10879029/diff/12002/net/tools/testserv...
net/tools/testserver/testserver.py:2152: os.chdir(MakeDataDir())
This strikes me as a little odd, just because it's not necessary for the other
tests.

[Takashi Toyoshima, 2012-08-31 17:38:20]

http://codereview.chromium.org/10879029/diff/12002/net/tools/testserver/run_t...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10879029/diff/12002/net/tools/testserver/run_t...
net/tools/testserver/run_testserver.cc:122: if (server_type ==
net::TestServer::TYPE_HTTP)
If no scheme option is specified, TYPE_HTTP is used as a default value.
I guess this line aims to support omitting scheme option for https.

WebSocket always requires scheme option, --ws or --wss.
So, auto type conversion looks confusing to me.
This is the reason why I didn't add WebSocket mapping here.

E.g., Handling "run_testserver --ssl-cert=ok" as https looks good, but handling
"run_testserver --ws --ssl-cert=ok" as wss looks curious. We may want to show
error message for this case. I feel that we should show warning on
"run_testserver --http --ssl-cert=ok", too. I revise this part to support proper
error on this.

http://codereview.chromium.org/10879029/diff/12002/net/tools/testserver/tests...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/10879029/diff/12002/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2152: os.chdir(MakeDataDir())
This is pywebsocket specific requirement.
It need to change current directory to document root.
I feel changing current directory is not so good manner.
So, I'll try to remove this by revising pywebsocket later.

[Takashi Toyoshima, 2012-08-31 17:43:58]

+akalin as a reviewer for python part.

Hi Akalin,
Ryan suggest me to add another python reviewer to land this change.
Could you review this change especially on testserver.py?

[akalin, 2012-08-31 19:50:12]

I think phajdan would be a better reviewer, as I'm only familiar with the
sync-specific bits of testserver.  He'll probably have something to say, anyway.

[Paweł Hajdan Jr., 2012-09-01 07:39:10]

http://codereview.chromium.org/10879029/diff/20001/net/test/local_test_server.cc
File net/test/local_test_server.cc (right):

http://codereview.chromium.org/10879029/diff/20001/net/test/local_test_server...
net/test/local_test_server.cc:179:
AppendToPythonPath(third_party_dir.AppendASCII("pywebsocket")
nit: Please follow line wrapping convention from above two lines if possible.

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
net/tools/testserver/run_testserver.cc:121: // If no scheme switch is specified,
select http or https scheme.
I don't like this implicit handling of flags. This also duplicates --http and
--https logic above.

It is preferable to throw an error rather than implicitly do something. Follow
the principle of least surprise.

[Takashi Toyoshima, 2012-09-01 08:36:59]

akalin:
Thank you for suggesting proper reviewer.

Paweł:
Thank you for review.
Ahead of revising this change again, I'd like to know your another opinion about
line 121.

BTW, I'm handling WebSocket test integration in bug 137639. I'll ask you some
other review on that. The goal of 137639 is removing old WebSocket server in
ui_test_utils which becomes zombie process sometimes.

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
net/tools/testserver/run_testserver.cc:121: // If no scheme switch is specified,
select http or https scheme.
This behavior is originally implemented in Line 104, 116-117 of the original
source. I'm afraid that removing this estimation may break some other dependent
tools. What do you think from the viewpoint of compatibility. If this was not
implemented originally, I'd totally agreed with your opinion.

[Paweł Hajdan Jr., 2012-09-04 09:28:01]

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10879029/diff/20001/net/tools/testserver/run_t...
net/tools/testserver/run_testserver.cc:121: // If no scheme switch is specified,
select http or https scheme.
On 2012/09/01 08:36:59, toyoshim wrote:
> This behavior is originally implemented in Line 104, 116-117 of the original
> source.

Ah, right - the new version just makes it more obvious (which is good).

> I'm afraid that removing this estimation may break some other dependent
> tools. What do you think from the viewpoint of compatibility. If this was not
> implemented originally, I'd totally agreed with your opinion.

Makes sense. I think run_testserver is just a tool for developers. Compatibility
shouldn't be a big concern here. Feel free to add a TODO here to make this CL
safer to land.

http://codereview.chromium.org/10879029/diff/29001/net/test/local_test_server.cc
File net/test/local_test_server.cc (right):

http://codereview.chromium.org/10879029/diff/29001/net/test/local_test_server...
net/test/local_test_server.cc:179:
AppendToPythonPath(third_party_dir.AppendASCII("pywebsocket")
nit: Please apply my suggestion from the previous round of comments.

[Takashi Toyoshima, 2012-09-11 06:36:31]

Hi Paweł,
Sorry for suspending this CL.
Now I revised again for landing.
Could you check this fix follow your comments?

https://chromiumcodereview.appspot.com/10879029/diff/20001/net/test/local_tes...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/20001/net/test/local_tes...
net/test/local_test_server.cc:179:
AppendToPythonPath(third_party_dir.AppendASCII("pywebsocket")
On 2012/09/01 07:39:10, Paweł Hajdan Jr. wrote:
> nit: Please follow line wrapping convention from above two lines if possible.

Done.

https://chromiumcodereview.appspot.com/10879029/diff/20001/net/tools/testserv...
File net/tools/testserver/run_testserver.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/20001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:121: // If no scheme switch is specified,
select http or https scheme.
On 2012/09/04 09:28:01, Paweł Hajdan Jr. wrote:
> On 2012/09/01 08:36:59, toyoshim wrote:
> > This behavior is originally implemented in Line 104, 116-117 of the original
> > source.
> 
> Ah, right - the new version just makes it more obvious (which is good).
> 
> > I'm afraid that removing this estimation may break some other dependent
> > tools. What do you think from the viewpoint of compatibility. If this was
not
> > implemented originally, I'd totally agreed with your opinion.
> 
> Makes sense. I think run_testserver is just a tool for developers.
Compatibility
> shouldn't be a big concern here. Feel free to add a TODO here to make this CL
> safer to land.

Done.

https://chromiumcodereview.appspot.com/10879029/diff/29001/net/test/local_tes...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/29001/net/test/local_tes...
net/test/local_test_server.cc:179:
AppendToPythonPath(third_party_dir.AppendASCII("pywebsocket")
On 2012/09/04 09:28:02, Paweł Hajdan Jr. wrote:
> nit: Please apply my suggestion from the previous round of comments.

Done.

[Paweł Hajdan Jr., 2012-09-12 10:32:55]

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/test/local_tes...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/test/local_tes...
net/test/local_test_server.cc:178: .AppendASCII("src"));
nit: Not really, I prefer the _previous_ wrapping style, i.e. without "hanging
dot". Something more like this:

AppendToPythonPath(
    third_party_dir.AppendASCII("pyftpdlib").AppendASCII("src"));

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
File net/tools/testserver/run_testserver.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:131: if (server_type ==
net::TestServer::TYPE_HTTP ||
This is inversion of UsingSSL. Please design things so it can be used here too.

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:162: case net::TestServer::TYPE_HTTPS:
This is also effectively UsingSSL.

[Takashi Toyoshima, 2012-09-13 07:06:23]

Thanks.
I revised again.

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/test/local_tes...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/test/local_tes...
net/test/local_test_server.cc:178: .AppendASCII("src"));
Ah, I see.
I took wrong way...

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
File net/tools/testserver/run_testserver.cc (right):

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:131: if (server_type ==
net::TestServer::TYPE_HTTP ||
OK. I believe no one want to use secure ftp, sync, and so on. So, I just use
!UsingSSL(type), here.
Done!

https://chromiumcodereview.appspot.com/10879029/diff/31001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:162: case net::TestServer::TYPE_HTTPS:
On 2012/09/12 10:32:55, Paweł Hajdan Jr. wrote:
> This is also effectively UsingSSL.

Done.

[Paweł Hajdan Jr., 2012-09-13 07:24:47]

LGTM

[commit-bot: I haz the power, 2012-09-13 08:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/26014

[commit-bot: I haz the power, 2012-09-13 08:56:55]

Try job failure for 10879029-26014 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-09-13 09:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/29009

[commit-bot: I haz the power, 2012-09-13 09:37:27]

Try job failure for 10879029-29009 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-09-13 10:14:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/29009

[commit-bot: I haz the power, 2012-09-13 10:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/29009

[commit-bot: I haz the power, 2012-09-13 11:17:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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, 2012-09-13 11:37:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/30010

[commit-bot: I haz the power, 2012-09-13 12:41:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/30010

[commit-bot: I haz the power, 2012-09-13 12:43:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/30010

[commit-bot: I haz the power, 2012-09-13 12:49:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/30010

[commit-bot: I haz the power, 2012-09-13 13:40:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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, 2012-09-14 03:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/30010

[commit-bot: I haz the power, 2012-09-14 04:22:49]

Change committed as 156742

[Takashi Toyoshima, 2012-10-05 12:37:44]

Patch Set 12 was landed, but reverted because of following Chromium OS failures.
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...

[Takashi Toyoshima, 2012-10-07 04:35:56]

https://gerrit.chromium.org/gerrit/34857
After the above change is landed, I'll try to reland this CL again.

[commit-bot: I haz the power, 2012-10-09 01:30:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10879029/48001

[commit-bot: I haz the power, 2012-10-09 03:43:21]

Change committed as 160782

[jochen (gone - plz use gerrit), 2012-10-11 09:16:37]

hey

this added a new dependency to chrome, but install-build-deps.sh was not updated
(nor was an update sent to chromium-dev asking developers to install it...)

:(

[Takashi Toyoshima, 2012-10-11 11:16:33]

pywebsocket is provided from src/third_party/pywebsocket.
So, I think I don't have to do something about install-build-deps.sh, and so on.