Asynchronously generate key pairs for <keygen> tag

Asynchronously generate key pairs for <keygen> tag

We currently hang up the IO thread.

R=wtc
BUG=148
TEST=verify that downloads, etc., are not interrupted by a <keygen>

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

[wtc, 2010-06-17 18:06:14]

I only reviewed the new unit test in keygen_handler_unittest.cc.
Just two nits below.  (I understand you cannot
check it in until we make the test pass.)

http://codereview.chromium.org/2854007/diff/1/5
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2854007/diff/1/5#newcode147
net/base/keygen_handler_unittest.cc:147: delete events[i];
Nit: set events[i] to NULL to avoid dangling pointers.

http://codereview.chromium.org/2854007/diff/1/5#newcode151
net/base/keygen_handler_unittest.cc:151: ASSERT_GT(results[i].length(), 0U);
Nit: Create a function for this verification code to share
with the SmokeTest.

[davidben, 2010-06-17 18:23:12]

Split off the unit test onto http://codereview.chromium.org/2838010 and
addressed comments.

On 2010/06/17 18:06:14, wtc wrote:
> I only reviewed the new unit test in keygen_handler_unittest.cc.
> Just two nits below.  (I understand you cannot
> check it in until we make the test pass.)
> 
> http://codereview.chromium.org/2854007/diff/1/5
> File net/base/keygen_handler_unittest.cc (right):
> 
> http://codereview.chromium.org/2854007/diff/1/5#newcode147
> net/base/keygen_handler_unittest.cc:147: delete events[i];
> Nit: set events[i] to NULL to avoid dangling pointers.
> 
> http://codereview.chromium.org/2854007/diff/1/5#newcode151
> net/base/keygen_handler_unittest.cc:151: ASSERT_GT(results[i].length(), 0U);
> Nit: Create a function for this verification code to share
> with the SmokeTest.

[wtc, 2010-06-25 00:21:11]

LGTM.  Thanks for the hard work behind this CL!

Note especially my last comment below.

http://codereview.chromium.org/2854007/diff/5001/6001
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1580
chrome/browser/renderer_host/resource_message_filter.cc:1580: const GURL& url,
Can we remove the |url| parameter?  It is not used.  Do you
know why it's there?  Was it in the WebKit API?

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1607
chrome/browser/renderer_host/resource_message_filter.cc:1607: NOTREACHED() <<
"Failed to dispatch keygen to worker pool";
Nit: keygen => keygen task

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1621
chrome/browser/renderer_host/resource_message_filter.cc:1621: // Generate a
signed key and challenge, then send it back.
Nit: key => public key

(That structure is called SPKAC, with a 'P' for "public".)

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1626
chrome/browser/renderer_host/resource_message_filter.cc:1626: Send(reply_msg);
Please make sure we can call the Send() method on a worker
thread.

[davidben, 2010-06-25 01:04:36]

http://codereview.chromium.org/2854007/diff/5001/6001
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1580
chrome/browser/renderer_host/resource_message_filter.cc:1580: const GURL& url,
On 2010/06/25 00:21:11, wtc wrote:
> Can we remove the |url| parameter?  It is not used.  Do you
> know why it's there?  Was it in the WebKit API?

 I think it's there because it's in the ViewHostMsg_Keygen message. It'll be
useful if we want to associate keys with URLs, so we might as well keep it. (At
the very least, we can set the user-visible name on the key in OS X like Safari
does.)

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1607
chrome/browser/renderer_host/resource_message_filter.cc:1607: NOTREACHED() <<
"Failed to dispatch keygen to worker pool";
On 2010/06/25 00:21:11, wtc wrote:
> Nit: keygen => keygen task

Done.

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1621
chrome/browser/renderer_host/resource_message_filter.cc:1621: // Generate a
signed key and challenge, then send it back.
On 2010/06/25 00:21:11, wtc wrote:
> Nit: key => public key
> 
> (That structure is called SPKAC, with a 'P' for "public".)

Done.

http://codereview.chromium.org/2854007/diff/5001/6001#newcode1626
chrome/browser/renderer_host/resource_message_filter.cc:1626: Send(reply_msg);
On 2010/06/25 00:21:11, wtc wrote:
> Please make sure we can call the Send() method on a worker
> thread.

Oh, good point. The rest of the code is making the IO thread do the send. Done.

[wtc, 2010-06-25 02:07:15]

LGTM.