Pulled a significant portion of Socket implementation into BaseSocket in order to prepare for the s…

Pulled a significant portion of Socket implementation into BaseSocket in order to prepare for the synchronous socket impementations which will also use these common methods.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/04229201426063e98a752f379b400f922761ed1b

[bkonyi, 2017-03-28 23:18:35]

bkonyi@google.com changed reviewers:
+ zra@google.com

[bkonyi, 2017-03-28 23:18:35]

https://codereview.chromium.org/2780063002/diff/1/runtime/bin/socket_common_m...
File runtime/bin/socket_common_macos.h (right):

https://codereview.chromium.org/2780063002/diff/1/runtime/bin/socket_common_m...
runtime/bin/socket_common_macos.h:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
For files that I've renamed, should I be updating the copyright date? My guess
is no, but it's worth asking.

[zra, 2017-03-29 02:37:26]

I landed changes to sockets this morning, so you'll need to rebase and merge
those in. Sorry!

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h#ne...
runtime/bin/socket.h:22: class Socket : public BaseSocket {
Since BaseSocket is all static, this doesn't do anything.

[bkonyi, 2017-03-29 04:09:28]

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h#ne...
runtime/bin/socket.h:22: class Socket : public BaseSocket {
On 2017/03/29 02:37:25, zra wrote:
> Since BaseSocket is all static, this doesn't do anything.

Yes, I know. This was more to show my intentions if that makes any sense?

[bkonyi, 2017-03-29 19:11:40]

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/2780063002/diff/20001/runtime/bin/socket.h#ne...
runtime/bin/socket.h:22: class Socket : public BaseSocket {
On 2017/03/29 02:37:25, zra wrote:
> Since BaseSocket is all static, this doesn't do anything.

Removed and added comment as discussed.

[zra, 2017-03-30 19:59:45]

High level comments:
1. BaseSocket -> SocketBase, socket_common -> socket_base.
2. Keep the socket_*.cc files for the Socket implementation. Don't put the
Socket implementation in socket_common_*.cc.
3. socket.cc should have only the Socket_ functions that are specific to async
sockets. socket_base.cc should have the Socket_ functions that are common to
both, renamed SocketBase_.
4. Some of the "common" functions currently allow an errno of EWOULDBLOCK, but
for a blocking socket, this would be unexpected.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/io_natives.cc
File runtime/bin/io_natives.cc (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/io_natives....
runtime/bin/io_natives.cc:128: V(BaseSocket_IsBindError, 2)                     
                           \
Please keep this list alphabetized.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.cc#n...
runtime/bin/socket.cc:31: ListeningSocketRegistry*
globalTcpListeningSocketRegistry = NULL;
Why move this?

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.h#ne...
runtime/bin/socket.h:12: #include "bin/socket_common.h"
Alphabetize.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_andr...
File runtime/bin/socket_android.cc (left):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_andr...
runtime/bin/socket_android.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
I think we should keep this file, and it should have the implementations of the
Socket:: functions. The Socket:: implementation shouldn't go in the
socket_common_*.cc files since they are not shared between sync and async
sockets.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
File runtime/bin/socket_common.h (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
runtime/bin/socket_common.h:250: class BaseSocket : public AllStatic {
Maybe call the file base_socket.h? Or since it would be nice if all socket code
was in files with socket_ names, maybe call this SocketCommon instead. Or call
this SocketBase, and the file files socket_base*.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
File runtime/bin/socket_common_android.cc (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
runtime/bin/socket_common_android.cc:48: Socket::Socket(intptr_t fd)
Socket:: implementation shouldn't be in here, I don't think. We probably need to
keep around e.g. socket_android.cc for these.

[bkonyi, 2017-04-01 23:41:18]

Please take a look when you have a chance.

Also, response for comment 4: I'll add a parameter when I submit the sync socket
code that will assert that specific error shouldn't occur.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/io_natives.cc
File runtime/bin/io_natives.cc (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/io_natives....
runtime/bin/io_natives.cc:128: V(BaseSocket_IsBindError, 2)                     
                           \
On 2017/03/30 19:59:44, zra wrote:
> Please keep this list alphabetized.

Done.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.cc#n...
runtime/bin/socket.cc:31: ListeningSocketRegistry*
globalTcpListeningSocketRegistry = NULL;
On 2017/03/30 19:59:44, zra wrote:
> Why move this?

I had moved it before I had decided to refactor, must have put it back in the
wrong place. Fixed.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket.h#ne...
runtime/bin/socket.h:12: #include "bin/socket_common.h"
On 2017/03/30 19:59:44, zra wrote:
> Alphabetize.

Done.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_andr...
File runtime/bin/socket_android.cc (left):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_andr...
runtime/bin/socket_android.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2017/03/30 19:59:44, zra wrote:
> I think we should keep this file, and it should have the implementations of
the
> Socket:: functions. The Socket:: implementation shouldn't go in the
> socket_common_*.cc files since they are not shared between sync and async
> sockets.

Done.

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
File runtime/bin/socket_common.h (right):

https://codereview.chromium.org/2780063002/diff/60001/runtime/bin/socket_comm...
runtime/bin/socket_common.h:250: class BaseSocket : public AllStatic {
On 2017/03/30 19:59:44, zra wrote:
> Maybe call the file base_socket.h? Or since it would be nice if all socket
code
> was in files with socket_ names, maybe call this SocketCommon instead. Or call
> this SocketBase, and the file files socket_base*.

Done.

[zra, 2017-04-02 15:51:16]

Looks like the socket_*.cc files still need to be added back to the CL

[bkonyi, 2017-04-02 17:43:33]

On 2017/04/02 15:51:16, zra wrote:
> Looks like the socket_*.cc files still need to be added back to the CL

It seems like the code review tool is just acting weird. If you click on the
socket_*.cc files you can see the modifications I made, even though the tool
says they're deleted files on the summary page. I double checked and the files
were in the commit as well. Please take another look when you have a chance :)

[zra, 2017-04-02 21:39:27]

Huh, weird.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/io_impl_so...
File runtime/bin/io_impl_sources.gypi (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/io_impl_so...
runtime/bin/io_impl_sources.gypi:71: 'socket_base.cc',
Alphabetize.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_and...
File runtime/bin/socket_android.cc (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_and...
runtime/bin/socket_android.cc:11: #include "bin/socket_base_android.h"
This should go down with the other bin/ includes.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_bas...
File runtime/bin/socket_base_unsupported.cc (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_bas...
runtime/bin/socket_base_unsupported.cc:30: void
FUNCTION_NAME(Socket_CreateConnect)(Dart_NativeArguments args) {
For consistency, these should also go back to socket_unsupported.cc

[bkonyi, 2017-04-02 22:09:29]

Addressed your comments. PTAL.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/io_impl_so...
File runtime/bin/io_impl_sources.gypi (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/io_impl_so...
runtime/bin/io_impl_sources.gypi:71: 'socket_base.cc',
On 2017/04/02 21:39:27, zra wrote:
> Alphabetize.

Done.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_and...
File runtime/bin/socket_android.cc (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_and...
runtime/bin/socket_android.cc:11: #include "bin/socket_base_android.h"
On 2017/04/02 21:39:27, zra wrote:
> This should go down with the other bin/ includes.

Done.

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_bas...
File runtime/bin/socket_base_unsupported.cc (right):

https://codereview.chromium.org/2780063002/diff/100001/runtime/bin/socket_bas...
runtime/bin/socket_base_unsupported.cc:30: void
FUNCTION_NAME(Socket_CreateConnect)(Dart_NativeArguments args) {
On 2017/04/02 21:39:27, zra wrote:
> For consistency, these should also go back to socket_unsupported.cc

Done.

[zra, 2017-04-03 03:40:22]

zra@google.com changed reviewers:
+ rmacnak@google.com

[zra, 2017-04-03 03:40:23]

+rmacnak for question about HOST_OS_ vs. TARGET_OS_.

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
File runtime/bin/socket_base.cc (right):

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
runtime/bin/socket_base.cc:8: #include "bin/socket.h"
Is this still needed? If so, it should be sorted down with the other bin/
includes.

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_base.h
File runtime/bin/socket_base.h (right):

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
runtime/bin/socket_base.h:14: #if defined(TARGET_OS_ANDROID)
I think these should probably still be HOST_OS_*

[rmacnak, 2017-04-03 16:03:56]

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_base.h
File runtime/bin/socket_base.h (right):

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
runtime/bin/socket_base.h:14: #if defined(TARGET_OS_ANDROID)
On 2017/04/03 03:40:23, zra wrote:
> I think these should probably still be HOST_OS_*

Right, HOST_OS. Only the AOT compiler should be using TARGET_OS.

[bkonyi, 2017-04-03 16:09:57]

Addressed comments. PTAL.

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
File runtime/bin/socket_base.cc (right):

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
runtime/bin/socket_base.cc:8: #include "bin/socket.h"
On 2017/04/03 03:40:23, zra wrote:
> Is this still needed? If so, it should be sorted down with the other bin/
> includes.

Nope, just an old include. Removed.

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_base.h
File runtime/bin/socket_base.h (right):

https://codereview.chromium.org/2780063002/diff/110001/runtime/bin/socket_bas...
runtime/bin/socket_base.h:14: #if defined(TARGET_OS_ANDROID)
On 2017/04/03 16:03:55, rmacnak wrote:
> On 2017/04/03 03:40:23, zra wrote:
> > I think these should probably still be HOST_OS_*
> 
> Right, HOST_OS. Only the AOT compiler should be using TARGET_OS.

Good to know! I don't know why/how these were changed in the first place... I'll
switch them back.

[zra, 2017-04-03 20:30:35]

lgtm with more include order nits.

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
File runtime/bin/socket_base_android.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
runtime/bin/socket_base_android.cc:10: #include "bin/socket.h"
If this is needed it should go with the other bin/ includes.

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
File runtime/bin/socket_base_fuchsia.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
runtime/bin/socket_base_fuchsia.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
File runtime/bin/socket_base_linux.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
runtime/bin/socket_base_linux.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
File runtime/bin/socket_base_macos.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
runtime/bin/socket_base_macos.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
File runtime/bin/socket_base_win.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_bas...
runtime/bin/socket_base_win.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_fuc...
File runtime/bin/socket_fuchsia.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_fuc...
runtime/bin/socket_fuchsia.cc:10: #include "bin/socket.h"
This isn't socket.cc, so socket.h should go down with the other bin/ includes.

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_lin...
File runtime/bin/socket_linux.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_lin...
runtime/bin/socket_linux.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_mac...
File runtime/bin/socket_macos.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_mac...
runtime/bin/socket_macos.cc:10: #include "bin/socket.h"
ditto

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/2780063002/diff/130001/runtime/bin/socket_win...
runtime/bin/socket_win.cc:10: #include "bin/socket.h"
ditto

[bkonyi, 2017-04-04 00:05:18]

Description was changed from

==========
Pulled a significant portion of Socket implementation into BaseSocket in order
to prepare for the synchronous socket impementations which will also use these
common methods.

BUG=
==========

to

==========
Pulled a significant portion of Socket implementation into BaseSocket in order
to prepare for the synchronous socket impementations which will also use these
common methods.

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/04229201426063e98a752f379b400f922761ed1b
==========

[bkonyi, 2017-04-04 00:05:21]

Committed patchset #9 (id:150001) manually as
04229201426063e98a752f379b400f922761ed1b (presubmit successful).