Chromium Code Reviews
Help | Chromium Project | Sign in
(105)

Issue 5958001: The MediatorThread worker thread needs to have a CertVerifier... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by wtc
Modified:
2 years, 11 months ago
Reviewers:
agl, akalin
CC:
chromium-reviews_chromium.org, Alpha
Visibility:
Public.

Description

The MediatorThread worker thread needs to have a CertVerifier
for the SSLClientSocket objects it creates.

R=agl,akalin
BUG=63357, 67239
TEST=Sync should not crash.

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

Patch Set 1 #

Patch Set 2 : Update comments #

Total comments: 2

Patch Set 3 : Change ChromeAsyncSocket constructor call in chrome_async_socket_unittest.cc #

Patch Set 4 : Fix XmppConnection constructor calls in unit tests #

Patch Set 5 : Fix typos in xmpp_connection_unittest.cc #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -28 lines) Lint Patch
M chrome/browser/sync/tools/sync_listen_notifications.cc View 1 2 3 4 chunks +8 lines, -3 lines 1 comment 0 errors Download
M jingle/notifier/base/chrome_async_socket.h View 1 2 3 chunks +5 lines, -2 lines 2 comments 0 errors Download
M jingle/notifier/base/chrome_async_socket.cc View 3 chunks +3 lines, -1 line 0 comments 0 errors Download
M jingle/notifier/base/chrome_async_socket_unittest.cc View 1 2 3 3 chunks +5 lines, -2 lines 0 comments 0 errors Download
M jingle/notifier/base/xmpp_connection.h View 1 2 2 chunks +6 lines, -0 lines 0 comments 0 errors Download
M jingle/notifier/base/xmpp_connection.cc View 4 chunks +6 lines, -3 lines 0 comments 0 errors Download
M jingle/notifier/base/xmpp_connection_unittest.cc View 1 2 3 4 9 chunks +12 lines, -10 lines 0 comments 0 errors Download
M jingle/notifier/communicator/login.h View 1 2 chunks +4 lines, -2 lines 0 comments 0 errors Download
M jingle/notifier/communicator/login.cc View 3 chunks +4 lines, -2 lines 0 comments 0 errors Download
M jingle/notifier/communicator/login_settings.h View 4 chunks +7 lines, -0 lines 1 comment 0 errors Download
M jingle/notifier/communicator/login_settings.cc View 4 chunks +5 lines, -1 line 0 comments 0 errors Download
M jingle/notifier/communicator/single_login_attempt.cc View 1 chunk +2 lines, -1 line 0 comments 0 errors Download
M jingle/notifier/listener/mediator_thread_impl.h View 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M jingle/notifier/listener/mediator_thread_impl.cc View 4 chunks +7 lines, -1 line 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 4
wtc
agl,akalin: please review. hclam: just FYI. Please review the files in this order, which reflects ...
3 years, 4 months ago #1
akalin
LGTM http://codereview.chromium.org/5958001/diff/13001/jingle/notifier/base/chrome_async_socket.h File jingle/notifier/base/chrome_async_socket.h (left): http://codereview.chromium.org/5958001/diff/13001/jingle/notifier/base/chrome_async_socket.h#oldcode39 jingle/notifier/base/chrome_async_socket.h:39: ChromeAsyncSocket(net::ClientSocketFactory* client_socket_factory, You'll need to change the constructor ...
3 years, 4 months ago #2
wtc
akalin: I addressed your review comments in Patch Set 5. Please review. (You can just ...
3 years, 4 months ago #3
agl
3 years, 4 months ago #4
LGTM

http://codereview.chromium.org/5958001/diff/48001/chrome/browser/sync/tools/s...
File chrome/browser/sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/5958001/diff/48001/chrome/browser/sync/tools/s...
chrome/browser/sync/tools/sync_listen_notifications.cc:302: 
(very minor): I would remove this blank line.

http://codereview.chromium.org/5958001/diff/48001/jingle/notifier/base/chrome...
File jingle/notifier/base/chrome_async_socket.h (right):

http://codereview.chromium.org/5958001/diff/48001/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.h:38: // Takes ownership of
|client_socket_factory| but not |cert_verifier| and
s/and/nor/

http://codereview.chromium.org/5958001/diff/48001/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.h:192: net::CertVerifier*
cert_verifier_;
I think this can be a const pointer if you like that sort of thing.

http://codereview.chromium.org/5958001/diff/48001/jingle/notifier/communicato...
File jingle/notifier/communicator/login_settings.h (right):

http://codereview.chromium.org/5958001/diff/48001/jingle/notifier/communicato...
jingle/notifier/communicator/login_settings.h:81: net::CertVerifier*
cert_verifier_;
likewise about the const pointer if you like them.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6