make SetReceiveBufferSize and SetSendBufferSize return net error codes (instead of bools)

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

[wtc, 2014-03-29 13:30:11]

Patch set 6 LGTM. Thanks a lot for writing this CL. I didn't reallize it would
be so big.

I suggested that we rename the error code
ERR_SOCKET_SET_RECEIVE_BUFFER_SIZE_ERROR to
ERR_SOCKET_RECEIVE_BUFFER_SIZE_UNCHANGEABLE, and similarly for the
send-buffer-size error. But I saw we need generic set-buffer-size errors in
jingle/glue/channel_socket_adapter.cc because the libjingle functions we call
only return a bool. So I wonder if we should instead *add* new error codes for
the Windows buffer-size-silently-unchanged condition
(ERR_SOCKET_RECEIVE_BUFFER_SIZE_NOT_CHANGED?). Using the existing error codes
for both cases is fine by me, too.

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

https://codereview.chromium.org/217573002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:183: return net::ERR_FAILED;

Change this to net::ERR_NOT_IMPLEMENTED, or change yours to net::ERR_FAILED for
consistency. (I prefer your choice.)

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_tcp_socket.cc (right):

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:271:
SendSetOptionACK(result ? PP_OK : PP_ERROR_FAILED);

Nit: You can ignore this suggestion.

  int result = net::ERR_UNEXPECTED;
  if (...) {
    ...
    result = tcp_socket->SetSendBufferSize(integer_value);
  } else {
    result = tcp_socket->SetReceiveBufferSize(integer_value);
  }
  SendSetOptionACK(result == net::OK ? PP_OK : PP_ERROR_FAILED);

Note that this method now has an opportunity to do error code mapping (from
net::ERR_xxx to PP_xxx). Maybe you can add a TODO comment.

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

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:468:
return result ? PP_OK : PP_ERROR_FAILED;

Nit: see my suggestion in the previous file.

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

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:157:
return result ? PP_OK : PP_ERROR_FAILED;

Nit: see my suggestion in the previous file.

https://codereview.chromium.org/217573002/diff/90001/jingle/glue/channel_sock...
File jingle/glue/channel_socket_adapter.cc (right):

https://codereview.chromium.org/217573002/diff/90001/jingle/glue/channel_sock...
jingle/glue/channel_socket_adapter.cc:105: net::OK :
net::ERR_SOCKET_SET_SEND_BUFFER_SIZE_ERROR;

BUG: the error code should be ...RECEIVE_BUFFER...

https://codereview.chromium.org/217573002/diff/90001/net/dns/address_sorter_p...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/217573002/diff/90001/net/dns/address_sorter_p...
net/dns/address_sorter_posix_unittest.cc:50: return OK;

I wonder why these two methods don't have NOTIMPLEMENTED().

https://codereview.chromium.org/217573002/diff/90001/net/spdy/spdy_proxy_clie...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/217573002/diff/90001/net/spdy/spdy_proxy_clie...
net/spdy/spdy_proxy_client_socket.cc:249: return ERR_METHOD_NOT_SUPPORTED;

IMPORTANT: Use ERR_NOT_IMPLEMENTED or some other error in these two methods.

ERR_METHOD_NOT_SUPPORTED is an HTTP error code. It means the server doesn't
support the HTTP method (GET, POST, etc.).

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

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:324: return MapSystemError(last_error);

This should be:
    return rv == 0 ? OK : MapSystemError(last_error);

The reason is that the value of errno is only trustworthy when a function fails.
A function is not required to set errno to 0 when it succeeds, so errno may
contain a stale error from a previous failed function call.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:333: return MapSystemError(last_error);

This should be: return rv == 0 ? OK : MapSystemError(last_error);

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

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:410: return ERR_SOCKET_SET_RECEIVE_BUFFER_SIZE_ERROR;

Please rename this error ERR_SOCKET_RECEIVE_BUFFER_SIZE_UNCHANGEABLE.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:429:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SocketUnchangeableSendBuffer",

Nit: the two histogram names are not symmetric:
  Net.SocketReceiveBufferUnchangeable
  Net.SocketUnchangeableSendBuffer

No need to change this.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:431: return ERR_SOCKET_SET_SEND_BUFFER_SIZE_ERROR;

Please rename this error ERR_SOCKET_SEND_BUFFER_SIZE_UNCHANGEABLE.

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

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/channe...
remoting/protocol/channel_multiplexer.cc:121: return net::ERR_NOT_IMPLEMENTED;

I also like net::ERR_NOT_IMPLEMENTED, but for consistency with the functions
below, let's use net::ERR_FAILED here and in SetSendBufferSize.

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/fake_s...
File remoting/protocol/fake_session.cc (right):

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/fake_s...
remoting/protocol/fake_session.cc:176: return net::ERR_FAILED;

Change to net::ERR_NOT_IMPLEMENTED for consistency, or change your methods to
net::ERR_FAILED.

[jar (doing other things), 2014-04-01 23:50:39]

wtc: IF I'm lucky... we'll see green bots.

I tried to separate the "Rebase" deltas from response to your comments... but at
least the comments are responded to individually below.

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

https://codereview.chromium.org/217573002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:183: return net::ERR_FAILED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Change this to net::ERR_NOT_IMPLEMENTED, or change yours to net::ERR_FAILED
for
> consistency. (I prefer your choice.)

Done.

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_tcp_socket.cc (right):

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:271:
SendSetOptionACK(result ? PP_OK : PP_ERROR_FAILED);
On 2014/03/29 13:30:12, wtc wrote:
> 
> Nit: You can ignore this suggestion.
> 
>   int result = net::ERR_UNEXPECTED;
>   if (...) {
>     ...
>     result = tcp_socket->SetSendBufferSize(integer_value);
>   } else {
>     result = tcp_socket->SetReceiveBufferSize(integer_value);
>   }
>   SendSetOptionACK(result == net::OK ? PP_OK : PP_ERROR_FAILED);
> 
> Note that this method now has an opportunity to do error code mapping (from
> net::ERR_xxx to PP_xxx). Maybe you can add a TODO comment.

I made the change, and added a TODO... but assigned to you.  If you know a
better target, I'll use him/her. thx!

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

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:468:
return result ? PP_OK : PP_ERROR_FAILED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Nit: see my suggestion in the previous file.

Done as in previous file... with TODO(wtc).

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

https://codereview.chromium.org/217573002/diff/90001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:157:
return result ? PP_OK : PP_ERROR_FAILED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Nit: see my suggestion in the previous file.

Done as in other file.

https://codereview.chromium.org/217573002/diff/90001/jingle/glue/channel_sock...
File jingle/glue/channel_socket_adapter.cc (right):

https://codereview.chromium.org/217573002/diff/90001/jingle/glue/channel_sock...
jingle/glue/channel_socket_adapter.cc:105: net::OK :
net::ERR_SOCKET_SET_SEND_BUFFER_SIZE_ERROR;
On 2014/03/29 13:30:12, wtc wrote:
> 
> BUG: the error code should be ...RECEIVE_BUFFER...

Done.

https://codereview.chromium.org/217573002/diff/90001/net/dns/address_sorter_p...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/217573002/diff/90001/net/dns/address_sorter_p...
net/dns/address_sorter_posix_unittest.cc:50: return OK;
On 2014/03/29 13:30:12, wtc wrote:
> 
> I wonder why these two methods don't have NOTIMPLEMENTED().

My presumption is that this is effectively a mock.

https://codereview.chromium.org/217573002/diff/90001/net/spdy/spdy_proxy_clie...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/217573002/diff/90001/net/spdy/spdy_proxy_clie...
net/spdy/spdy_proxy_client_socket.cc:249: return ERR_METHOD_NOT_SUPPORTED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> IMPORTANT: Use ERR_NOT_IMPLEMENTED or some other error in these two methods.
> 
> ERR_METHOD_NOT_SUPPORTED is an HTTP error code. It means the server doesn't
> support the HTTP method (GET, POST, etc.).

Done.

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

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:324: return MapSystemError(last_error);
On 2014/03/29 13:30:12, wtc wrote:
> 
> This should be:
>     return rv == 0 ? OK : MapSystemError(last_error);
> 
> The reason is that the value of errno is only trustworthy when a function
fails.
> A function is not required to set errno to 0 when it succeeds, so errno may
> contain a stale error from a previous failed function call.

Done.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:333: return MapSystemError(last_error);
On 2014/03/29 13:30:12, wtc wrote:
> 
> This should be: return rv == 0 ? OK : MapSystemError(last_error);

Done.

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

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:410: return ERR_SOCKET_SET_RECEIVE_BUFFER_SIZE_ERROR;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Please rename this error ERR_SOCKET_RECEIVE_BUFFER_SIZE_UNCHANGEABLE.

Done.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:429:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SocketUnchangeableSendBuffer",
On 2014/03/29 13:30:12, wtc wrote:
> 
> Nit: the two histogram names are not symmetric:
>   Net.SocketReceiveBufferUnchangeable
>   Net.SocketUnchangeableSendBuffer
> 
> No need to change this.
>   

Yes... this was a mistake I made, where I updated teh XML, and not (one of) the
C++ code entries.  I later went back and set the XML to match this non-symmetric
naming, so that we would get data sooner.

Now that we have basic data, and I can change one and obsolete the old one.

https://codereview.chromium.org/217573002/diff/90001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:431: return ERR_SOCKET_SET_SEND_BUFFER_SIZE_ERROR;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Please rename this error ERR_SOCKET_SEND_BUFFER_SIZE_UNCHANGEABLE.

Done.

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

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/channe...
remoting/protocol/channel_multiplexer.cc:121: return net::ERR_NOT_IMPLEMENTED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> I also like net::ERR_NOT_IMPLEMENTED, but for consistency with the functions
> below, let's use net::ERR_FAILED here and in SetSendBufferSize.

I switched the others to ERR_NOT_IMPLEMENTED, which seems much more correct.

I also correct the IsConnected() to return false (since it can't even connect!)

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/fake_s...
File remoting/protocol/fake_session.cc (right):

https://codereview.chromium.org/217573002/diff/90001/remoting/protocol/fake_s...
remoting/protocol/fake_session.cc:176: return net::ERR_FAILED;
On 2014/03/29 13:30:12, wtc wrote:
> 
> Change to net::ERR_NOT_IMPLEMENTED for consistency, or change your methods to
> net::ERR_FAILED.

Done.

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

https://codereview.chromium.org/217573002/diff/160001/jingle/glue/pseudotcp_a...
jingle/glue/pseudotcp_adapter.cc:498: return net::OK;
 Is this fix a bug.

[wtc, 2014-04-02 00:52:29]

Patch set 11 LGTM.

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

https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:42: return rv;

IMPORTANT: these two functions should return like this:

  return rv == 0 ? OK : MapSystemError(GetLastError());

[jar (doing other things), 2014-04-02 01:18:32]

wtc: Perhaps this will green up Linux as well ;-)

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

https://codereview.chromium.org/217573002/diff/180001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:42: return rv;
On 2014/04/02 00:52:29, wtc wrote:
> 
> IMPORTANT: these two functions should return like this:
> 
>   return rv == 0 ? OK : MapSystemError(GetLastError());

Done.

Also made corresponding changes in the Linux file.

[wtc, 2014-04-02 03:23:30]

Patch set 12 LGTM. You had a typo.

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

https://codereview.chromium.org/217573002/diff/200001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:508: return (rv == 0) ? OK :
MapSysteError(errno);

Typo: Syste => System

Also on line 515.

[jar (doing other things), 2014-04-02 22:08:01]

The CQ bit was checked by jar@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001

[commit-bot: I haz the power, 2014-04-02 22:36:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:36:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jar (doing other things), 2014-04-02 23:04:44]

This was already pretty carefully reviewed by wtc@... but it touches a bunch of
files :-(.

yurys: Please review chrome/browser/devtools/adb/*
sergeyu: Please review  content/browser/renderer_host/p2p/*
          and remoting/*
yzshen: Please review  content/browser/renderer_host/pepper/*
zea: Please review jingle/*

Thanks in advance!

(p.s.,: For extra credit... be the last reviewer AND hit the CQ button)

[yurys, 2014-04-03 09:09:23]

chrome/browser/devtools/adb/* - LGTM

[Nicolas Zea, 2014-04-03 17:46:45]

jingle lgtm

[jar (doing other things), 2014-04-04 19:12:32]

The CQ bit was checked by jar@chromium.org

[commit-bot: I haz the power, 2014-04-04 19:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001

[commit-bot: I haz the power, 2014-04-04 19:54:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 19:54:22]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jar (doing other things), 2014-04-04 20:43:30]

The CQ bit was checked by jar@chromium.org

[commit-bot: I haz the power, 2014-04-04 20:48:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001

[commit-bot: I haz the power, 2014-04-04 21:29:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/217573002/220001

[commit-bot: I haz the power, 2014-04-05 03:52:26]

Change committed as 261966

[Ryan Hamilton, 2014-04-08 19:55:11]

Here are the straws I was able to grasp which *might* (but probably don't :>)
explain the test failure.

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;
One more change in semantics here. Again, almost assuredly not important.

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;
FYI, you could use ERR_UNEXPECTED here since you expect that it's *definitely*
overwritten in the if () {} else {}

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;
ditto about ERR_UNEXPECTED

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;
One more instance of a semantic change here.

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);
please DCHECK_NE(0, errno)

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);
please DCHECK_NE(0, errno)

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());
Possibly again here with the map error 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());
we could consider doing the DCHECK here too.

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);
one more here (errno == 0, potential gotcha)

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());
possibly here too.

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;
Note: this is actually a minor semantic change.  Seems fine, but if we're
grasping at straws, here's a straw :>

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

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

[jar (doing other things), 2014-04-08 23:16:25]

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...