Added pepper test for SSLHandshake

Added a pepper test for SSLHandshake. This starts an SSL server which the test can connect to.

BUG=114626
TEST=Ran pepper TCP tests.


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

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

[yzshen1, 2012-03-21 22:37:57]

Thanks for working on this.
It is great to see more tests. :)

https://chromiumcodereview.appspot.com/9791003/diff/11001/chrome/test/ui/ppap...
File chrome/test/ui/ppapi_uitest.cc (right):

https://chromiumcodereview.appspot.com/9791003/diff/11001/chrome/test/ui/ppap...
chrome/test/ui/ppapi_uitest.cc:183: RunTestViaHTTP(url);
nit:
It might be better to have a private method named
RunHTTPTestServer(web_dir, test_case, extra_params)
and then let RunTestViaHTTP() and RunTestWithSSLServer() call it.

So that we can save a call to GetHTTPDocumentRoot(), and it is more clear about
the extra parameter.

https://chromiumcodereview.appspot.com/9791003/diff/11001/chrome/test/ui/ppap...
chrome/test/ui/ppapi_uitest.cc:835:
TEST_PPAPI_OUT_OF_PROCESS_WITH_SSL_SERVER(TCPSocketPrivate);
Please update this accordingly once you check in the other CL of removing
TCPSocketPrivateShared.

[raymes, 2012-03-21 23:33:06]

http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:183: RunTestViaHTTP(url);
On 2012/03/21 22:37:58, yzshen1 wrote:
> nit:
> It might be better to have a private method named
> RunHTTPTestServer(web_dir, test_case, extra_params)
> and then let RunTestViaHTTP() and RunTestWithSSLServer() call it.
> 
> So that we can save a call to GetHTTPDocumentRoot(), and it is more clear
about
> the extra parameter.

Hmm, I'm not sure exactly what you mean here. Can you give a more concrete
example of what you are suggesting?

http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:835:
TEST_PPAPI_OUT_OF_PROCESS_WITH_SSL_SERVER(TCPSocketPrivate);
On 2012/03/21 22:37:58, yzshen1 wrote:
> Please update this accordingly once you check in the other CL of removing
> TCPSocketPrivateShared.

Done.

[yzshen1, 2012-03-21 23:42:11]

http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:183: RunTestViaHTTP(url);
The current code calls RunTestViaHTTP() directly, as a result,
- GetHTTPDocumentRoot is called twice (one in RunTestWithSSLServer and the other
in RunTestViaHTTP)
- the way to add ssl_server_port is not very elegant. The code actually pretends
that it is part of the test_case name.

So, maybe the following is better:
void RunTestViaHTTP(test_case) {
  ...
  GetHTTPDocumentRoot(&web_dir);
  ...
  RunHTTPTestServer(web_dir, test_case, "");
}

void RunTestWithSSLServer(test_case) {
  ...
  GetHTTPDocumentRoot(&web_dir);
  // Start the ssl server.
  ...
  RunHTTPTestServer(web_dir, test_case, "ssl_server_port=1234");
}

Does that make sense?
On 2012/03/21 23:33:07, raymes wrote:
> On 2012/03/21 22:37:58, yzshen1 wrote:
> > nit:
> > It might be better to have a private method named
> > RunHTTPTestServer(web_dir, test_case, extra_params)
> > and then let RunTestViaHTTP() and RunTestWithSSLServer() call it.
> > 
> > So that we can save a call to GetHTTPDocumentRoot(), and it is more clear
> about
> > the extra parameter.
> 
> Hmm, I'm not sure exactly what you mean here. Can you give a more concrete
> example of what you are suggesting?

[raymes, 2012-03-22 00:39:46]

On 2012/03/21 23:42:11, yzshen1 wrote:
>
http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest.cc
> File chrome/test/ui/ppapi_uitest.cc (right):
> 
>
http://codereview.chromium.org/9791003/diff/11001/chrome/test/ui/ppapi_uitest...
> chrome/test/ui/ppapi_uitest.cc:183: RunTestViaHTTP(url);
> The current code calls RunTestViaHTTP() directly, as a result,
> - GetHTTPDocumentRoot is called twice (one in RunTestWithSSLServer and the
other
> in RunTestViaHTTP)
> - the way to add ssl_server_port is not very elegant. The code actually
pretends
> that it is part of the test_case name.
> 
> So, maybe the following is better:
> void RunTestViaHTTP(test_case) {
>   ...
>   GetHTTPDocumentRoot(&web_dir);
>   ...
>   RunHTTPTestServer(web_dir, test_case, "");
> }
> 
> void RunTestWithSSLServer(test_case) {
>   ...
>   GetHTTPDocumentRoot(&web_dir);
>   // Start the ssl server.
>   ...
>   RunHTTPTestServer(web_dir, test_case, "ssl_server_port=1234");
> }
> 
> Does that make sense?
> On 2012/03/21 23:33:07, raymes wrote:
> > On 2012/03/21 22:37:58, yzshen1 wrote:
> > > nit:
> > > It might be better to have a private method named
> > > RunHTTPTestServer(web_dir, test_case, extra_params)
> > > and then let RunTestViaHTTP() and RunTestWithSSLServer() call it.
> > > 
> > > So that we can save a call to GetHTTPDocumentRoot(), and it is more clear
> > about
> > > the extra parameter.
> > 
> > Hmm, I'm not sure exactly what you mean here. Can you give a more concrete
> > example of what you are suggesting?

That makes sense. Thanks. I tried to address this in this next CL.

[yzshen1, 2012-03-22 00:54:58]

lgtm

http://codereview.chromium.org/9791003/diff/6010/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9791003/diff/6010/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:225: if (extra_params.length() > 0)
nit: if (!extra_params.empty())

[commit-bot: I haz the power, 2012-03-22 17:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9791003/17017

[commit-bot: I haz the power, 2012-03-22 19:09:37]

Change committed as 128266

[commit-bot: I haz the power, 2012-03-22 23:18:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9791003/23002

[commit-bot: I haz the power, 2012-03-23 01:31:53]

Change committed as 128374