net: Add NET_API for some linux-only files

net: Add NET_API to more code to enable building
a shared net library on Linux, and update base and
crypto API definitions.

BUG=76997
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91234

[rvargas (doing something else), 2011-06-24 02:19:29]

Evan,

ProxyConfigServiceLinux was annotated with BASE_API, but that looks like a
mistake.

[Evan Martin, 2011-06-24 02:32:55]

LGTM, but I think maybe one file is a mistake

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.h:86: NET_TEST void FlushSocketPools();
This is a bit weird.  How can you even call this method if the ctor isn't also
tagged NET_TEST?

[wtc, 2011-06-24 16:36:26]

LGTM.

Nit: some of these files are not Linux only; they are also
used by Mac OS X.

http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h
File net/base/net_api.h (right):

http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h#newcode29
net/base/net_api.h:29: #endif
I suggest adding some blank lines to make the structure of
this nested ifdef clearer.

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.h:86: NET_TEST void FlushSocketPools();
On 2011/06/24 02:32:55, Evan Martin wrote:
> This is a bit weird.  How can you even call this method if the ctor isn't also
> tagged NET_TEST?

I guess the constructor is called by some other NET_TEST
function, and our unit tests only create this object using
that function?

http://codereview.chromium.org/7240021/diff/1/net/socket/tcp_client_socket_li...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/7240021/diff/1/net/socket/tcp_client_socket_li...
net/socket/tcp_client_socket_libevent.h:26: base::NonThreadSafe {
Should we also add 'public' to base::NonThreadSafe?

[wtc, 2011-06-24 16:38:50]

http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h
File net/base/net_api.h (right):

http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h#newcode24
net/base/net_api.h:24: #define NET_TEST __attribute__((visibility("default")))
I guess there is a corresponding GYP file change to compile
with -fvisibility=hidden?

[Evan Martin, 2011-06-24 17:59:37]

On 2011/06/24 16:38:50, wtc wrote:
> http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h
> File net/base/net_api.h (right):
> 
> http://codereview.chromium.org/7240021/diff/1/net/base/net_api.h#newcode24
> net/base/net_api.h:24: #define NET_TEST __attribute__((visibility("default")))
> I guess there is a corresponding GYP file change to compile
> with -fvisibility=hidden?

Yeah, we build with that by default.

[rvargas (doing something else), 2011-06-24 22:37:20]

Thanks.

I added http_network_session.*, base_api.h and crypto_api.h.

Please take another look.

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/7240021/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.h:86: NET_TEST void FlushSocketPools();
Good point. The problem was that HttpNetworkSession::CloseAllConnections was
inlined, so the linker was not able to see FlushSocketPools even though it was
not really intended to be called directly by net_unittests. Note that msvc
doesn't inline methods of exported classes.

[wtc, 2011-06-24 23:18:48]

LGTM.

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h#newcode21
base/base_api.h:21: 
These blank lines don't work for me either, so please
remove them.  Sorry about that.

Perhaps only the use of indentation will help, but we
don't indent ifdefs in the Chromium code base.

The comments you added do help though.  Thanks.

[rvargas (doing something else), 2011-06-24 23:27:58]

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h#newcode21
base/base_api.h:21: 
Actually the lines help me (although they look weird here with green lines).

I'd prefer to keep them unless you guys think it is better without them.

On 2011/06/24 23:18:48, wtc wrote:
> These blank lines don't work for me either, so please
> remove them.  Sorry about that.
> 
> Perhaps only the use of indentation will help, but we
> don't indent ifdefs in the Chromium code base.
> 
> The comments you added do help though.  Thanks.

[wtc, 2011-06-24 23:44:18]

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h#newcode21
base/base_api.h:21: 
On 2011/06/24 23:27:58, rvargas wrote:
> Actually the lines help me (although they look weird here with green lines).
> 
> I'd prefer to keep them unless you guys think it is better without them.

OK! :-)

I'm afraid that only indentation would work for me:

#if defined(BASE_DLL)
#  if defined(WIN32)
#    if defined(BASE_IMPLEMENTATION)
#    define BASE_API __declspec(dllexport)
#    else
#    define BASE_API __declspec(dllimport)
#    endif  // defined(BASE_IMPLEMENTATION)
#  else  // defined(WIN32)
#    define BASE_API __attribute__((visibility("default")))
#  endif
#else  // defined(BASE_DLL)
#  define BASE_API
#endif

[rvargas (doing something else), 2011-06-25 01:16:45]

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h#newcode21
base/base_api.h:21: 
On 2011/06/24 23:44:19, wtc wrote:
> On 2011/06/24 23:27:58, rvargas wrote:
> > Actually the lines help me (although they look weird here with green lines).
> > 
> > I'd prefer to keep them unless you guys think it is better without them.
> 
> OK! :-)
> 
> I'm afraid that only indentation would work for me:
> 
> #if defined(BASE_DLL)
> #  if defined(WIN32)
> #    if defined(BASE_IMPLEMENTATION)
> #    define BASE_API __declspec(dllexport)
> #    else
> #    define BASE_API __declspec(dllimport)
> #    endif  // defined(BASE_IMPLEMENTATION)
> #  else  // defined(WIN32)
> #    define BASE_API __attribute__((visibility("default")))
> #  endif
> #else  // defined(BASE_DLL)
> #  define BASE_API
> #endif

yes, indenting will be the most clear, but we don't do that... and it has the
disadvantage of failing on a search for "#define foo"

I need an approval for base anyway... so, Brett, wdyt?

[brettw, 2011-06-30 21:17:33]

base LGTM

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/7240021/diff/8001/base/base_api.h#newcode21
base/base_api.h:21: 
We don't normally do the indenting. I think in this case with your blank lines
it's good enough. If it was much more complicated we'd have to do some kind of
indenting or it would be totally unreadable.