Issue 2126023002: use std::unique_ptr for the callback argument of QUIC's ProofVerifier::Verify proof to make ownersh…

Issue 2126023002: use std::unique_ptr for the callback argument of QUIC's ProofVerifier::Verify proof to make ownersh… (Closed)

Created:
4 years, 5 months ago by rjshade

Modified:
4 years, 5 months ago

Reviewers:
Ryan Hamilton

CC:
chromium-reviews, cbentzel+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@126327823

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

use std::unique_ptr for the callback argument of QUIC's ProofVerifier::Verify proof to make ownership clear. Merge internal change: 126331456 R=rch@chromium.org BUG=

Patch Set 1 #

Total comments: 2

Created: 4 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+38 lines, -38 lines)			Patch
M	net/quic/crypto/crypto_server_test.cc	View	1 chunk	+3 lines, -2 lines	0 comments	Download
M	net/quic/crypto/proof_test.cc	View	1 chunk	+2 lines, -1 line	0 comments	Download
M	net/quic/crypto/proof_verifier.h	View	2 chunks	+1 line, -2 lines	0 comments	Download
M	net/quic/crypto/proof_verifier_chromium.h	View	1 chunk	+1 line, -1 line	0 comments	Download
M	net/quic/crypto/proof_verifier_chromium.cc	View	5 chunks	+5 lines, -6 lines	2 comments	Download
M	net/quic/crypto/proof_verifier_chromium_test.cc	View	11 chunks	+11 lines, -11 lines	0 comments	Download
M	net/quic/quic_crypto_client_stream.cc	View	2 chunks	+2 lines, -3 lines	0 comments	Download
M	net/tools/quic/test_tools/quic_test_client.cc	View	1 chunk	+13 lines, -12 lines	0 comments	Download

Depends on Patchset:

Issue 2128043002 Patch 1

Dependent Patchsets:

Issue 2124093002 Patch 1

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

rjshade

https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verifier_chromium.cc File net/quic/crypto/proof_verifier_chromium.cc (left): https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verifier_chromium.cc#oldcode186 net/quic/crypto/proof_verifier_chromium.cc:186: DCHECK(callback); Not sure about this removal, but the following ...

4 years, 5 months ago (2016-07-06 20:09:34 UTC) #2

Ryan Hamilton

4 years, 5 months ago (2016-07-07 00:10:35 UTC) #3

rjshade

On 2016/07/07 00:10:35, Ryan Hamilton wrote: > https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verifier_chromium.cc > File net/quic/crypto/proof_verifier_chromium.cc (left): > > https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verifier_chromium.cc#oldcode186 ...

4 years, 5 months ago (2016-07-07 15:24:45 UTC) #4

Ryan Hamilton

4 years, 5 months ago (2016-07-07 16:30:04 UTC) #5

On 2016/07/07 15:24:45, rjshade wrote:
> On 2016/07/07 00:10:35, Ryan Hamilton wrote:
> >
>
https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verif...
> > File net/quic/crypto/proof_verifier_chromium.cc (left):
> > 
> >
>
https://codereview.chromium.org/2126023002/diff/1/net/quic/crypto/proof_verif...
> > net/quic/crypto/proof_verifier_chromium.cc:186: DCHECK(callback);
> > On 2016/07/06 20:09:34, rjshade wrote:
> > > Not sure about this removal, but the following test fails:
> > > 
> > > VerifyECDSAKnownAnswerTest
> > > https://cs.chromium.org/chromium/src/net/quic/crypto/proof_test.cc?l=329
> > > 
> > > with it. Any suggestions?
> > 
> > Hm. That sounds dodgy. DO you have a failure log?
> 
> It was a difference in proof_test.cc Chromium vs internal. See patchset in
Final
> merge CL: https://codereview.chromium.org/2132623002

LGTM. Thanks!

Expand Messages | Collapse Messages