Add an async ChannelIDSource for testing.

Add an async ChannelIDSource for testing.

R=avd@chromium.org,rch@chromium.org
BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282253

[wtc, 2014-07-09 01:03:53]

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
File net/quic/test_tools/crypto_test_utils.cc (left):

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
net/quic/test_tools/crypto_test_utils.cc:241: LOG(INFO) << "here";

This looks like a debugging message that Adam forgot to delete. So I deleted it.

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
net/quic/test_tools/crypto_test_utils.h:42: class TaskSource {

I'd appreciate suggestion for a better name.

If the term "task" is not used elsewhere in the QUIC source code, I can use
"callback" instead of "task".

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
net/quic/test_tools/crypto_test_utils.h:91: TaskSource* task_source = NULL);

It seems that the Style Guide allows this use of default argument:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

This is a static method, and we may need to add additional TaskSource arguments
(to simulate a variable-length argument list) in the future, for example, if we
want to use an async ProofVerifier for testing.

[Ryan Hamilton, 2014-07-09 16:54:39]

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
net/quic/test_tools/crypto_test_utils.h:91: TaskSource* task_source = NULL);
On 2014/07/09 01:03:52, wtc wrote:
> 
> It seems that the Style Guide allows this use of default argument:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
> 
> This is a static method, 

The way I read the style guide, it's not just that the method is static, it's
that it's in a .cc file not  .h file:

  One specific exception is when the function is a static function (or in an
unnamed namespace) in a .cc file. In this case, the cons don't apply since the
function's use is so localized.

> and we may need to add additional TaskSource arguments
> (to simulate a variable-length argument list) in the future, for example, if
we
> want to use an async ProofVerifier for testing.

If we need to do more in the future we can figure it out then. In the meantime,
it doesn't seem like it's actually doing multiple arguments. I think that having
2 different methods (in which one simply invokes the other with NULL for then
last arg) would be most consistent with existing code.

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.cc (right):

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.cc:129: // to GetChannelIDKey is NULL.
nit: can you wrap this onto the previous line?

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:41: // An interface for a source of
tasks to run.
This is a very generic description. Can you add comments which explain in more
details what subclasses of this method would do? I assume it's going to do
something like MessageLoop::RunUntilIdle() or EpollServer::ExecuteCallbacks (or
whatever that's called)?

EDIT: after reading the rest of the code, my guess was totally wrong :>

[wtc, 2014-07-09 19:44:42]

Ryan: thank you for the review. Please review patch set 3.

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/1/net/quic/test_tools/crypto_t...
net/quic/test_tools/crypto_test_utils.h:91: TaskSource* task_source = NULL);

On 2014/07/09 16:54:39, Ryan Hamilton wrote:
> I think that having
> 2 different methods (in which one simply invokes the other with NULL for then
> last arg) would be most consistent with existing code.

Done. Added a CommunicateHandshakeMessagesAndDoWork method.

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.cc (right):

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.cc:129: // to GetChannelIDKey is NULL.

On 2014/07/09 16:54:39, Ryan Hamilton wrote:
> nit: can you wrap this onto the previous line?

Done. The first line was folded by "git cl format". I didn't realize it doesn't
add the excess to the second line.

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:41: // An interface for a source of
tasks to run.

On 2014/07/09 16:54:39, Ryan Hamilton wrote:
> This is a very generic description. Can you add comments which explain in more
> details what subclasses of this method would do?

Done. Added more comments. Also renamed this class "WorkSource" and the method
"DoPendingWork". I avoided "Task" or "Callback" in the names so that the names
don't suggest the use of MessageLoop or EpollServer.

[Ryan Hamilton, 2014-07-09 22:55:49]

lgtm

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:42: // to do the work from this source.
Can you mentioned that this may be use for things like invoking callbacks
asynchronously, or some such?

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:103: WorkSource* work_source = NULL);
I suspect that you meant to remove the = NULL here.

[wtc, 2014-07-09 23:21:48]

Ryan: thank you for the review! I am submitting the CL to the commit queue.

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils.h (right):

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:42: // to do the work from this source.

On 2014/07/09 22:55:49, Ryan Hamilton wrote:
> Can you mentioned that this may be use for things like invoking callbacks
> asynchronously, or some such?

Done.

https://codereview.chromium.org/380543003/diff/40001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils.h:103: WorkSource* work_source = NULL);

On 2014/07/09 22:55:49, Ryan Hamilton wrote:
> I suspect that you meant to remove the = NULL here.

Done. Thank you for catching this!

[wtc, 2014-07-09 23:21:55]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:23:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wtc@chromium.org/380543003/100001

[commit-bot: I haz the power, 2014-07-10 04:30:38]

Change committed as 282253