Issue 1171893003: Adding DCHECKs and constness to DtlsIdentityStore.

Issue 1171893003: Adding DCHECKs and constness to DtlsIdentityStore. (Closed)

Created:
5 years, 6 months ago by tommi

Modified:
5 years, 6 months ago

Reviewers:
hbos

CC:
Andrew MacDonald, interface-changes_webrtc.org, niklas.enbom, qiang.lu, rwolff_gocast.it, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc)

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

Adding DCHECKs and constness to DtlsIdentityStore. R=hbos@webrtc.org, hbos BUG= Committed: https://chromium.googlesource.com/external/webrtc/+/532caeae2d3e8c043f61f72d2f909c081ce2e6b4

Patch Set 2 : Address comments and add more const #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+19 lines, -11 lines)			Patch
	M	talk/app/webrtc/dtlsidentitystore.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
	M	talk/app/webrtc/dtlsidentitystore.cc	View	1	7 chunks	+17 lines, -9 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

hbos

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitystore.cc File talk/app/webrtc/dtlsidentitystore.cc (right): https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitystore.cc#newcode78 talk/app/webrtc/dtlsidentitystore.cc:78: case MSG_GENERATE_IDENTITY: You could add a DCHECK here that ...

5 years, 6 months ago (2015-06-09 12:59:12 UTC) #2

tommi

5 years, 6 months ago (2015-06-09 13:17:18 UTC) #4

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:78: case MSG_GENERATE_IDENTITY:
On 2015/06/09 at 12:59:12, hbos wrote:
> You could add a DCHECK here that we are on the worker thread if you add a
worker thread param to the WorkerTask constructor.

I thought about that but decided to not add another member variable for now.

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:85: case MSG_DESTROY:
On 2015/06/09 at 12:59:11, hbos wrote:
> And here that we are on the signaling thread.

Done.  There also is one in the dtor.

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:86: delete msg->pdata;
On 2015/06/09 at 12:59:11, hbos wrote:
> Add a comment that this line will delete |this| to clarify that one must not
touch member variables after this line.

Done

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:142: void
DtlsIdentityStore::OnMessage(rtc::Message* msg) {
On 2015/06/09 at 12:59:11, hbos wrote:
> And here that we are on the signaling thread for both MSG cases.

Done.

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:164: void
DtlsIdentityStore::GenerateIdentity() {
On 2015/06/09 at 12:59:11, hbos wrote:
> It's confusing that we have two GenerateIdentity in the same file, and
figuring out what is going on requires staring at the code for a bit. Can we
rename one of them for clarity?
> 
> Perhaps this can be called StartGenerateIdentityOnWorkerThread() if that's not
too long of a name. Or with a less descriptive name perhaps add a comment.

I added the _w() post fix to the one that runs on the worker thread.  That seems
to be the convention in this file.  Also made most methods of WorkerTask
private.

https://codereview.webrtc.org/1171893003/diff/1/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:219: GenerateIdentity();
On 2015/06/09 at 12:59:12, hbos wrote:
> (This generates in case we want to request another identity in the future,
right?)

yes.  I guess that's what the "free identity" reference in the comment refers
to.

tommi

5 years, 6 months ago (2015-06-09 15:33:24 UTC) #6

Message was sent while issue was closed.

Committed patchset #2 (id:20001) manually as
532caeae2d3e8c043f61f72d2f909c081ce2e6b4 (presubmit successful).

Expand Messages | Collapse Messages