Use std::function instead of base::Callback in net/http2/.

Use std::function instead of base::Callback in net/http2/.

This CL also lands server change 143046389 by bnc.

BUG=488484
Review-Url: https://codereview.chromium.org/2572343002
Cr-Commit-Position: refs/heads/master@{#442895}
Committed: https://chromium.googlesource.com/chromium/src/+/317ae39493cd0af9b8fd2bf19804744d487e8412

[Bence, 2016-12-15 00:13:18]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 00:13:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 00:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:31:19]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Bence, 2016-12-15 13:14:48]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 13:15:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 13:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 13:37:21]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-12-15 13:40:15]

bnc@chromium.org changed reviewers:
+ jamessynge@chromium.org

[Bence, 2016-12-15 13:40:16]

James: PTAL.  Thank you.

[Bence, 2016-12-27 20:00:12]

Description was changed from

==========
Use std::function instead of base::Callback in net/http2/.
==========

to

==========
Use std::function instead of base::Callback in net/http2/.

This CL also lands server change 143046389 by bnc.

BUG=488484
==========

[James Synge, 2017-01-05 21:39:05]

lgtm

This is a great change, it really reduces the number of diffs enormously. Thanks
so much for pushing through the change to allow std::function in tests.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/decod...
File net/http2/decoder/decode_buffer_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/decod...
net/http2/decoder/decode_buffer_test.cc:221: // Ensure high-bit is set.
Remove comment, it is on the next line.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/http2...
File net/http2/decoder/http2_frame_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/http2...
net/http2/decoder/http2_frame_decoder_test.cc:142: Validator validator =
[&expected, this](
I'm curious why "auto validator" became "Validator validator", especially as you
did keep "auto validator" in some other locations (see line 186 below in the
patch set).

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
File net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h
(right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h:150: if
(SupportedFrameType) {
I don't recall if we discussed this, but why is this block not:

    EXPECT_EQ(SupportedFrameType, IsSupportedHttp2FrameType(frame_type))
        << frame_type;

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h:202:
NoArgValidator validator = [&expected, this]() -> AssertionResult {
auto validator?

https://codereview.chromium.org/2572343002/diff/20001/net/http2/hpack/decoder...
File net/http2/hpack/decoder/hpack_block_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/hpack/decoder...
net/http2/hpack/decoder/hpack_block_decoder_test.cc:88: NoArgValidator do_check
= [this]() {
Maybe: auto do_check = ... 

Or maybe we should change the original to match this, to reduce "arbitrary"
diffs.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
File net/http2/tools/random_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
net/http2/tools/random_decoder_test.cc:105: if (original_remaining <= 30) {
Given the replacement of the test-only flag with this magic number, perhaps we
should at least add a member for getting the value so that we can provide
commentary and shared code except where the value is produced.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
File net/http2/tools/random_decoder_test.h (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
net/http2/tools/random_decoder_test.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
There are a bunch of minor comment differences in this file vs. the original,
but it isn't clear why that should be.

[Bence, 2017-01-06 01:42:39]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2017-01-06 01:42:40]

Ryan: PTAL.  Thanks.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/decod...
File net/http2/decoder/decode_buffer_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/decod...
net/http2/decoder/decode_buffer_test.cc:221: // Ensure high-bit is set.
On 2017/01/05 21:39:06, James Synge wrote:
> Remove comment, it is on the next line.

Done.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/http2...
File net/http2/decoder/http2_frame_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/http2...
net/http2/decoder/http2_frame_decoder_test.cc:142: Validator validator =
[&expected, this](
On 2017/01/05 21:39:06, James Synge wrote:
> I'm curious why "auto validator" became "Validator validator", especially as
you
> did keep "auto validator" in some other locations (see line 186 below in the
> patch set).

If I use auto here, I get the compile error below.  I do not understand why.  I
specified Validator where necessary, left auto elsewhere.

../../net/http2/decoder/http2_frame_decoder_test.cc:150:18: error: call to
'ValidateDoneAndEmpty' is ambiguous
        payload, ValidateDoneAndEmpty(validator)));
                 ^~~~~~~~~~~~~~~~~~~~
../../net/http2/tools/failure.h:142:56: note: expanded from macro
'VERIFY_SUCCESS'
  if (::testing::AssertionResult __assertion_result = (expr)) \
                                                       ^~~~
../../net/http2/tools/random_decoder_test.h:182:20: note: candidate function
  static Validator ValidateDoneAndEmpty(const Validator& wrapped) {
                   ^
../../net/http2/tools/random_decoder_test.h:193:20: note: candidate function
  static Validator ValidateDoneAndEmpty(NoArgValidator wrapped) {
                   ^
../../net/http2/decoder/http2_frame_decoder_test.cc:162:18: error: call to
'ValidateDoneAndOffset' is ambiguous
        payload, ValidateDoneAndOffset(payload.size(), validator)));
                 ^~~~~~~~~~~~~~~~~~~~~
../../net/http2/tools/failure.h:142:56: note: expanded from macro
'VERIFY_SUCCESS'
  if (::testing::AssertionResult __assertion_result = (expr)) \
                                                       ^~~~
../../net/http2/tools/random_decoder_test.h:215:20: note: candidate function
  static Validator ValidateDoneAndOffset(uint32_t offset, Validator wrapped) {
                   ^
../../net/http2/tools/random_decoder_test.h:226:20: note: candidate function
  static Validator ValidateDoneAndOffset(uint32_t offset,
                   ^
2 errors generated.
ninja: build stopped: subcommand failed.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
File net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h
(right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h:150: if
(SupportedFrameType) {
On 2017/01/05 21:39:06, James Synge wrote:
> I don't recall if we discussed this, but why is this block not:
> 
>     EXPECT_EQ(SupportedFrameType, IsSupportedHttp2FrameType(frame_type))
>         << frame_type;

This was changed server side at change 134543154.  But unfortunately I do not
remember why this had to be so contrived.  It compiles for me locally.  Let's
revisit this in a separate CL.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/decoder/paylo...
net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h:202:
NoArgValidator validator = [&expected, this]() -> AssertionResult {
On 2017/01/05 21:39:06, James Synge wrote:
> auto validator?

../../net/http2/decoder/payload_decoders/payload_decoder_base_test_util.h:208:18:
error: call to 'ValidateDoneAndEmpty' is ambiguous
        payload, ValidateDoneAndEmpty(validator));
                 ^~~~~~~~~~~~~~~~~~~~
../../net/http2/decoder/payload_decoders/priority_payload_decoder_test.cc:89:17:
note: in instantiation of member function
'net::test::AbstractPayloadDecoderTest<net::PriorityPayloadDecoder,
net::test::PriorityPayloadDecoderPeer, net::test::(anonymous
namespace)::Listener, true>::DecodePayloadAndValidateSeveralWays' requested here
    EXPECT_TRUE(DecodePayloadAndValidateSeveralWays(fb.buffer(), expected));
                ^
../../net/http2/tools/random_decoder_test.h:182:20: note: candidate function
  static Validator ValidateDoneAndEmpty(const Validator& wrapped) {
                   ^
../../net/http2/tools/random_decoder_test.h:193:20: note: candidate function
  static Validator ValidateDoneAndEmpty(NoArgValidator wrapped) {
                   ^
1 error generated.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/hpack/decoder...
File net/http2/hpack/decoder/hpack_block_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/hpack/decoder...
net/http2/hpack/decoder/hpack_block_decoder_test.cc:88: NoArgValidator do_check
= [this]() {
On 2017/01/05 21:39:06, James Synge wrote:
> Maybe: auto do_check = ... 
> 
> Or maybe we should change the original to match this, to reduce "arbitrary"
> diffs.

I meant to do that in change 143046389, but overlooked this line.  Sorry.

This should be solved by fixing the TODO at
https://cs.corp.google.com/piper///depot/google3/gfe/http2/tools/random_decod...
though, so Chromium port can be changed to |auto| at that point.

../../net/http2/hpack/decoder/hpack_block_decoder_test.cc:103:48: error: call to
'ValidateDoneAndEmpty' is ambiguous
                                               ValidateDoneAndEmpty(do_check)));
                                               ^~~~~~~~~~~~~~~~~~~~
../../testing/gtest/include/gtest/gtest.h:1864:23: note: expanded from macro
'EXPECT_TRUE'
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \
                      ^~~~~~~~~
../../testing/gtest/include/gtest/internal/gtest-internal.h:1189:34: note:
expanded from macro 'GTEST_TEST_BOOLEAN_'
      ::testing::AssertionResult(expression)) \
                                 ^~~~~~~~~~
../../net/http2/tools/random_decoder_test.h:182:20: note: candidate function
  static Validator ValidateDoneAndEmpty(const Validator& wrapped) {
                   ^
../../net/http2/tools/random_decoder_test.h:193:20: note: candidate function
  static Validator ValidateDoneAndEmpty(NoArgValidator wrapped) {
                   ^
1 error generated.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
File net/http2/tools/random_decoder_test.cc (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
net/http2/tools/random_decoder_test.cc:105: if (original_remaining <= 30) {
On 2017/01/05 21:39:06, James Synge wrote:
> Given the replacement of the test-only flag with this magic number, perhaps we
> should at least add a member for getting the value so that we can provide
> commentary and shared code except where the value is produced.

That sound like a good idea.  I'll do that once all internal patches are merged
to Chromium.

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
File net/http2/tools/random_decoder_test.h (right):

https://codereview.chromium.org/2572343002/diff/20001/net/http2/tools/random_...
net/http2/tools/random_decoder_test.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2017/01/05 21:39:06, James Synge wrote:
> There are a bunch of minor comment differences in this file vs. the original,
> but it isn't clear why that should be.

I am not sure which revision I was using when preparing this change.  I will
definitely revisit differences once Chromium code is brought up-to-date with
internal code.

[Ryan Hamilton, 2017-01-10 22:48:58]

lgtm

[Bence, 2017-01-11 13:44:20]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-01-11 13:44:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamessynge@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2572343002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-01-11 13:44:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 14:15:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484142260303230,
"parent_rev": "c38084e12ff9707e2cc07ff82e004980c43c99c9", "commit_rev":
"317ae39493cd0af9b8fd2bf19804744d487e8412"}

[commit-bot: I haz the power, 2017-01-11 14:15:50]

Description was changed from

==========
Use std::function instead of base::Callback in net/http2/.

This CL also lands server change 143046389 by bnc.

BUG=488484
==========

to

==========
Use std::function instead of base::Callback in net/http2/.

This CL also lands server change 143046389 by bnc.

BUG=488484

Review-Url: https://codereview.chromium.org/2572343002
Cr-Commit-Position: refs/heads/master@{#442895}
Committed:
https://chromium.googlesource.com/chromium/src/+/317ae39493cd0af9b8fd2bf19804...
==========

[commit-bot: I haz the power, 2017-01-11 14:15:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/317ae39493cd0af9b8fd2bf19804...