Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(72)

Issue 217573002: make SetReceiveBufferSize and SetSendBufferSize return net error codes (instead of bools) (Closed)

Created:
6 years, 9 months ago by jar (doing other things)
Modified:
6 years, 8 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, vsevik, chromoting-reviews_chromium.org, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, mmenke, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman
Visibility:
Public.

Description

make SetReceiveBufferSize and SetSendBufferSize return net error codes (instead of bools) TBR=sergeyu,yzshen R=wtc BUG=355222 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261966

Patch Set 1 #

Patch Set 2 : Fix open_ssl declaration and definition #

Patch Set 3 : fix fake_ssl_client, pseudotcp_adpter, and address_sorter unittests #

Patch Set 4 : fix up some remoting and p2p usages and declarations/definitions #

Patch Set 5 : Fix second (sneaky) pair of setters in fake_session.* #

Patch Set 6 : process error code in quic_stream_factory #

Total comments: 28

Patch Set 7 : rebase #

Patch Set 8 : Respond to wtc comments #

Patch Set 9 : Adjust a comment, and remove an extraneous "fix" that may have broken a try bot #

Patch Set 10 : rebase again #

Total comments: 1

Patch Set 11 : Fix one more bool-->int return (in a local function) and put back tiny bug fix #

Total comments: 2

Patch Set 12 : Map Windows error code, and add correspondin changes in Linux file #

Total comments: 1

Patch Set 13 : fix typo on Linux #

Total comments: 26
Unified diffs Side-by-side diffs Delta from patch set Stats (+281 lines, -245 lines) Patch
M chrome/browser/devtools/adb/android_usb_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/devtools/adb/android_usb_socket.cc View 1 2 3 4 5 6 7 2 chunks +5 lines, -5 lines 2 comments Download
M content/browser/renderer_host/p2p/socket_host_tcp.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.h View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.cc View 1 2 3 1 chunk +5 lines, -4 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_udp.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_udp_unittest.cc View 1 2 3 1 chunk +4 lines, -4 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_tcp_socket.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -4 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -4 lines 2 comments Download
M content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -4 lines 2 comments Download
M jingle/glue/channel_socket_adapter.h View 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/channel_socket_adapter.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -4 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket_unittest.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/proxy_resolving_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/proxy_resolving_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -4 lines 0 comments Download
M jingle/glue/pseudotcp_adapter.h View 1 chunk +2 lines, -2 lines 0 comments Download
M jingle/glue/pseudotcp_adapter.cc View 1 chunk +4 lines, -4 lines 2 comments Download
M jingle/glue/pseudotcp_adapter_unittest.cc View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M net/base/net_error_list.h View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -2 lines 0 comments Download
M net/dns/address_sorter_posix_unittest.cc View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M net/dns/mock_mdns_socket_factory.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/http/http_proxy_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/http/http_proxy_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/quic_stream_factory.cc View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -4 lines 0 comments Download
M net/socket/buffered_write_stream_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/buffered_write_stream_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/client_socket_pool_base_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/socket.h View 1 chunk +4 lines, -4 lines 0 comments Download
M net/socket/socket_test_util.h View 3 chunks +6 lines, -6 lines 0 comments Download
M net/socket/socket_test_util.cc View 3 chunks +12 lines, -12 lines 0 comments Download
M net/socket/socks5_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/socks5_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/socks_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/socks_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_nss.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_nss.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.h View 1 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_server_socket_nss.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_server_socket_nss.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_server_socket_unittest.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M net/socket/tcp_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_socket_libevent.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_socket_libevent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -10 lines 4 comments Download
M net/socket/tcp_socket_win.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_socket_win.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +10 lines, -8 lines 4 comments Download
M net/socket/transport_client_socket_pool_unittest.cc View 3 chunks +6 lines, -6 lines 0 comments Download
M net/spdy/spdy_proxy_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/spdy/spdy_proxy_client_socket.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -6 lines 0 comments Download
M net/udp/datagram_server_socket.h View 1 chunk +4 lines, -2 lines 0 comments Download
M net/udp/udp_client_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/udp/udp_client_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/udp/udp_server_socket.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/udp/udp_server_socket.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/udp/udp_socket_libevent.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/udp/udp_socket_libevent.cc View 1 2 3 4 5 6 7 1 chunk +8 lines, -6 lines 2 comments Download
M net/udp/udp_socket_win.h View 1 chunk +4 lines, -2 lines 0 comments Download
M net/udp/udp_socket_win.cc View 1 2 3 4 5 6 7 1 chunk +31 lines, -28 lines 2 comments Download
M remoting/jingle_glue/chromium_socket_factory.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M remoting/protocol/channel_multiplexer.cc View 1 2 3 4 5 6 7 9 10 2 chunks +8 lines, -8 lines 6 comments Download
M remoting/protocol/fake_session.h View 1 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
M remoting/protocol/fake_session.cc View 1 2 3 4 5 6 7 3 chunks +11 lines, -9 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +11 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
wtc
Patch set 6 LGTM. Thanks a lot for writing this CL. I didn't reallize it ...
6 years, 8 months ago (2014-03-29 13:30:11 UTC) #1
jar (doing other things)
wtc: IF I'm lucky... we'll see green bots. I tried to separate the "Rebase" deltas ...
6 years, 8 months ago (2014-04-01 23:50:39 UTC) #2
wtc
Patch set 11 LGTM. https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_win.cc#newcode42 net/socket/tcp_socket_win.cc:42: return rv; IMPORTANT: these two ...
6 years, 8 months ago (2014-04-02 00:52:29 UTC) #3
jar (doing other things)
wtc: Perhaps this will green up Linux as well ;-) https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_win.cc#newcode42 ...
6 years, 8 months ago (2014-04-02 01:18:32 UTC) #4
wtc
Patch set 12 LGTM. You had a typo. https://codereview.chromium.org/217573002/diff/200001/net/socket/tcp_socket_libevent.cc File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/217573002/diff/200001/net/socket/tcp_socket_libevent.cc#newcode508 net/socket/tcp_socket_libevent.cc:508: return ...
6 years, 8 months ago (2014-04-02 03:23:30 UTC) #5
jar (doing other things)
The CQ bit was checked by jar@chromium.org
6 years, 8 months ago (2014-04-02 22:08:01 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001
6 years, 8 months ago (2014-04-02 22:09:00 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-02 22:36:43 UTC) #8
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=59162
6 years, 8 months ago (2014-04-02 22:36:44 UTC) #9
jar (doing other things)
This was already pretty carefully reviewed by wtc@... but it touches a bunch of files ...
6 years, 8 months ago (2014-04-02 23:04:44 UTC) #10
yurys
chrome/browser/devtools/adb/* - LGTM
6 years, 8 months ago (2014-04-03 09:09:23 UTC) #11
Nicolas Zea
jingle lgtm
6 years, 8 months ago (2014-04-03 17:46:45 UTC) #12
jar (doing other things)
The CQ bit was checked by jar@chromium.org
6 years, 8 months ago (2014-04-04 19:12:32 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001
6 years, 8 months ago (2014-04-04 19:13:45 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-04 19:54:22 UTC) #15
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=59696
6 years, 8 months ago (2014-04-04 19:54:22 UTC) #16
jar (doing other things)
The CQ bit was checked by jar@chromium.org
6 years, 8 months ago (2014-04-04 20:43:30 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001
6 years, 8 months ago (2014-04-04 20:48:11 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001
6 years, 8 months ago (2014-04-04 21:29:27 UTC) #19
commit-bot: I haz the power
Change committed as 261966
6 years, 8 months ago (2014-04-05 03:52:26 UTC) #20
Ryan Hamilton
Here are the straws I was able to grasp which *might* (but probably don't :>) ...
6 years, 8 months ago (2014-04-08 19:55:11 UTC) #21
jar (doing other things)
6 years, 8 months ago (2014-04-08 23:16:25 UTC) #22
Message was sent while issue was closed.
Ryan: Thanks for looking and suggesting things!!!!

See the CL https://codereview.chromium.org/227473008/ for the implementation of
the changes you suggested (as per comments below).

I'll probably end up taking more and more of your conservative suggestions if I
can't get a landing to stick.

https://codereview.chromium.org/217573002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/adb/android_usb_socket.cc (right):

https://codereview.chromium.org/217573002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/adb/android_usb_socket.cc:183: return
net::ERR_NOT_IMPLEMENTED;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> One more change in semantics here. Again, almost assuredly not important.

I'm going to hold off (not do) this change unless my next landing fails.  This
shouldn't be able to hurt a debug build since the NOTIMPLEMENTED() protects the
code.

https://codereview.chromium.org/217573002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/217573002/diff/220001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:458:
int net_result = net::OK;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> FYI, you could use ERR_UNEXPECTED here since you expect that it's *definitely*
> overwritten in the if () {} else {}

Done.

https://codereview.chromium.org/217573002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/217573002/diff/220001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:143:
int net_result = net::OK;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> ditto about ERR_UNEXPECTED

Done.

https://codereview.chromium.org/217573002/diff/220001/jingle/glue/pseudotcp_a...
File jingle/glue/pseudotcp_adapter.cc (right):

https://codereview.chromium.org/217573002/diff/220001/jingle/glue/pseudotcp_a...
jingle/glue/pseudotcp_adapter.cc:491: return net::OK;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> One more instance of a semantic change here.

I *think* I understand the call sites (which were not checking the result!), so
I'll stick with this code for my first re-landing.

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:508: return (rv == 0) ? OK :
MapSystemError(errno);
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> please DCHECK_NE(0, errno)

Per discussion... I put in a local method to handle the (hopefully) unlikely
errno == 0 event.

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:515: return (rv == 0) ? OK :
MapSystemError(errno);
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> please DCHECK_NE(0, errno)

handled as above.

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_w...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:34: int net_error = (rv == 0) ? OK :
MapSystemError(WSAGetLastError());
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> Possibly again here with the map error code.

Since I'm not seeing any problems on Windows... I'll assume they are
compliant... and can use this simpler/standard code.

https://codereview.chromium.org/217573002/diff/220001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:42: int net_error = (rv == 0) ? OK :
MapSystemError(WSAGetLastError());
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> we could consider doing the DCHECK here too.

See above.

https://codereview.chromium.org/217573002/diff/220001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/217573002/diff/220001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:324: return rv == 0 ? OK :
MapSystemError(last_error);
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> one more here (errno == 0, potential gotcha)

Handled in common function.

https://codereview.chromium.org/217573002/diff/220001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/217573002/diff/220001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:396: return MapSystemError(WSAGetLastError());
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> possibly here too.

Since Windows has not shown any problems, I'd rather not bother with the extra
(non-standard) mapping handler here.

https://codereview.chromium.org/217573002/diff/220001/remoting/protocol/chann...
File remoting/protocol/channel_multiplexer.cc (right):

https://codereview.chromium.org/217573002/diff/220001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.cc:130: return net::ERR_NOT_IMPLEMENTED;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> Note: this is actually a minor semantic change.  Seems fine, but if we're
> grasping at straws, here's a straw :>

I'm going to hold off on this preservation unless the first landing attempt
fails.  Also note that this change is protected by a NOTIMPLEMENTED (and we're
seeing trouble in a debug build).

https://codereview.chromium.org/217573002/diff/220001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.cc:145: return net::ERR_NOT_IMPLEMENTED;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> Note: this is actually a minor semantic change.  Seems fine, but if we're
> grasping at straws, here's a straw :>

Holding off...

https://codereview.chromium.org/217573002/diff/220001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.cc:149: return net::ERR_NOT_IMPLEMENTED;
On 2014/04/08 19:55:11, Ryan Hamilton wrote:
> Note: this is actually a minor semantic change.  Seems fine, but if we're
> grasping at straws, here's a straw :>


Holding off...

Powered by Google App Engine
This is Rietveld 408576698