Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(80)

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

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

Description

make SetReceiveBufferSize and SetSendBufferSize return net error codes (instead of bools) Previously committed as https://src.chromium.org/viewvc/chrome?view=rev&revision=261966 but then reverted when it broke the Mac 10.6 build. TBR=sergeyu,yzshen,yurys,zea R=wtc BUG=355222 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262652

Patch Set 1 : Original CL, as was submitted, and previously reverted #

Total comments: 6

Patch Set 2 : Remove a "bug fix" that *might* have caused revert; fix one more bool vs int test #

Patch Set 3 : Remove typo and (should-be) fix from channel_multiplexer.cc #

Patch Set 4 : Added suggestions made by Ryan #

Patch Set 5 : fix linux typo #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+300 lines, -246 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 2 chunks +5 lines, -5 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_tcp.cc View 1 2 chunks +4 lines, -4 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.h View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.cc View 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 chunk +4 lines, -4 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_tcp_socket.cc View 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 1 chunk +5 lines, -4 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc View 1 2 3 1 chunk +5 lines, -4 lines 0 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 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 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 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 0 comments Download
M jingle/glue/pseudotcp_adapter_unittest.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M net/base/net_error_list.h View 1 chunk +10 lines, -2 lines 0 comments Download
M net/base/net_errors.h View 1 2 3 1 chunk +7 lines, -0 lines 1 comment Download
M net/base/net_errors.cc View 1 2 3 1 chunk +9 lines, -0 lines 2 comments Download
M net/dns/address_sorter_posix_unittest.cc View 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 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 chunk +2 lines, -2 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.cc View 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 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_socket_libevent.cc View 1 2 3 4 1 chunk +8 lines, -10 lines 0 comments Download
M net/socket/tcp_socket_win.h View 1 chunk +2 lines, -2 lines 0 comments Download
M net/socket/tcp_socket_win.cc View 2 chunks +10 lines, -8 lines 0 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 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 chunk +8 lines, -6 lines 0 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 chunk +31 lines, -28 lines 0 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 2 chunks +7 lines, -7 lines 0 comments Download
M remoting/protocol/fake_session.h View 2 chunks +4 lines, -4 lines 0 comments Download
M remoting/protocol/fake_session.cc View 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: 35 (0 generated)
jar (doing other things)
Hopefully, this CL, without the attempted-fix in channel_multiplexer.cc, will do better. https://codereview.chromium.org/227473008/diff/1/jingle/glue/pseudotcp_adapter.cc File jingle/glue/pseudotcp_adapter.cc (right): ...
6 years, 4 months ago (2014-04-08 16:11:22 UTC) #1
wtc
Patch set 3 LGTM. https://codereview.chromium.org/227473008/diff/1/jingle/glue/pseudotcp_adapter.cc File jingle/glue/pseudotcp_adapter.cc (right): https://codereview.chromium.org/227473008/diff/1/jingle/glue/pseudotcp_adapter.cc#newcode491 jingle/glue/pseudotcp_adapter.cc:491: return net::OK; On 2014/04/08 16:11:22, ...
6 years, 4 months ago (2014-04-08 18:44:38 UTC) #2
jar (doing other things)
https://codereview.chromium.org/227473008/diff/1/jingle/glue/pseudotcp_adapter.cc File jingle/glue/pseudotcp_adapter.cc (right): https://codereview.chromium.org/227473008/diff/1/jingle/glue/pseudotcp_adapter.cc#newcode491 jingle/glue/pseudotcp_adapter.cc:491: return net::OK; On 2014/04/08 18:44:38, wtc wrote: > > ...
6 years, 4 months ago (2014-04-08 23:14:10 UTC) #3
jar (doing other things)
The CQ bit was checked by jar@chromium.org
6 years, 4 months ago (2014-04-09 01:39:19 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 01:54:18 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 02:04:45 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/227473008/70001
6 years, 4 months ago (2014-04-09 02:22:45 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 04:41:06 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 04:46:42 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 04:52:05 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 04:56:53 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 06:42:15 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 06:49:34 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/227473008/70001
6 years, 4 months ago (2014-04-09 06:55:47 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:02:35 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:08:48 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:16:06 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/227473008/70001
6 years, 4 months ago (2014-04-09 07:22:36 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/227473008/70001
6 years, 4 months ago (2014-04-09 07:28:38 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:36:59 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:44:08 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:51:08 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 07:58:50 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 08:05:30 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 08:12:59 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:05:20 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:13:38 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:21:04 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:28:52 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:40:43 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 09:49:54 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 11:09:47 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/227473008/70001
6 years, 4 months ago (2014-04-09 11:21:30 UTC) #33
commit-bot: I haz the power
Change committed as 262652
6 years, 4 months ago (2014-04-09 12:21:17 UTC) #34
wtc
6 years, 4 months ago (2014-04-09 15:34:48 UTC) #35
Message was sent while issue was closed.
Patch set 5 LGTM. Thanks!

I can take care of fixing the nits.

https://codereview.chromium.org/227473008/diff/70001/net/base/net_errors.cc
File net/base/net_errors.cc (right):

https://codereview.chromium.org/227473008/diff/70001/net/base/net_errors.cc#n...
net/base/net_errors.cc:66: DLOG(WARNING)  << "OS Error was not set meaningfully
" << os_error;

Hmm... the problem with this error message is that it doesn't have a context.
Here are some ideas to provide a context.

1. Also log default_net_error.

2. Add a "failed_function_name" argument to the function.

3. Dump a call stack.

Nit: "meaningfully" seems superfluous. "Error" doesn't need to be capitalized.

https://codereview.chromium.org/227473008/diff/70001/net/base/net_errors.cc#n...
net/base/net_errors.cc:67: DCHECK_NE(default_net_error, OK);

This DCHECK should be at the beginning of the function, so that it is always
executed.

https://codereview.chromium.org/227473008/diff/70001/net/base/net_errors.h
File net/base/net_errors.h (right):

https://codereview.chromium.org/227473008/diff/70001/net/base/net_errors.h#ne...
net/base/net_errors.h:65: int MapSystemErrorWithDefault(int os_error, int
default_net_error);

Nit: please list this function right below the closely related MapSystemError.

Powered by Google App Engine
This is Rietveld 408576698