net: Add NET_API to net/base

net: Add NET_API to net/base

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

[wtc, 2011-05-16 20:55:17]

LGTM.  Thanks!

High-Level Comments

1. Please try to find a better solution for MSVC warning 4275
for NonThreadSafe, or add more comments to explain the current
solution.

2. I prefer just removing the #ifdef UNIT_TEST.

3. I commented on a few NET_TESTs that I think should be
NET_APIs.

http://codereview.chromium.org/6969077/diff/1/base/threading/non_thread_safe.h
File base/threading/non_thread_safe.h (right):

http://codereview.chromium.org/6969077/diff/1/base/threading/non_thread_safe....
base/threading/non_thread_safe.h:10: #include "base/compiler_specific.h"
We discussed this issue in person.

Your current solution is fine, but we should explain in this
file what warning 4275 is (at least note it's a MSVC warning)
and how one can fix the compiler warning.

A problem with including "base/compiler_specific.h" in this
file is that this file itself doesn't use
"base/compiler_specific.h".  I understand why you do it this
way after looking at the other files, but I was wondering
about it when I first saw it.

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

http://codereview.chromium.org/6969077/diff/1/net/base/cert_verify_result.h#n...
net/base/cert_verify_result.h:18: class NET_TEST CertVerifyResult {
CertVerifyResult is used by CertVerifier.

If CertVerifier is NET_API, CertVerifyResult should also be
NET_API.

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_mapping_rules.h#n...
net/base/host_mapping_rules.h:18: class NET_TEST HostMappingRules {
I seem to remember there are command-line switches that use
HostMappingRules.  So perhaps this should be NET_API?

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_port_pair.h#newco...
net/base/host_port_pair.h:18: class NET_TEST HostPortPair {
I suspect HostPortPair is used outside src/net, so it probably
should be NET_API?

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_resolver_impl.h#n...
net/base/host_resolver_impl.h:63: class NET_API HostResolverImpl
Can this be NET_TEST?  "Impl" seems to imply it is an internal
class.

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

http://codereview.chromium.org/6969077/diff/1/net/base/net_log.h#newcode284
net/base/net_log.h:284: class NET_TEST ScopedNetLogEvent {
This kind of scoped class seems useful, so probably should
be NET_API?

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

http://codereview.chromium.org/6969077/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.h:97: #if defined(UNIT_TEST) ||
defined(NET_DLL)
Perhaps we should just delete this ifdef, given our discussion
with jar before that UNIT_TEST is a bad idea?

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

http://codereview.chromium.org/6969077/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:66: #if defined(UNIT_TEST) || defined(NET_DLL)
Perhaps we should just remove this ifdef?

Could you explain why NET_DLL also needs this method?

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

http://codereview.chromium.org/6969077/diff/1/net/base/x509_cert_types.h#newc...
net/base/x509_cert_types.h:49: struct NET_TEST CertPrincipal {
CertPrincipal should be NET_API because
X509Certificate::subject() and X509Certificate::issuer()
returns a CertPrincipal.

[rvargas (doing something else), 2011-05-16 22:41:55]

I added compiler_specific.h to the CL, and defined a new macro , please see if
that's ok.

http://codereview.chromium.org/6969077/diff/1/base/threading/non_thread_safe.h
File base/threading/non_thread_safe.h (right):

http://codereview.chromium.org/6969077/diff/1/base/threading/non_thread_safe....
base/threading/non_thread_safe.h:10: #include "base/compiler_specific.h"
I updated the comment to be more explicit about the #include, and to point out
the macro that I'm adding.


On 2011/05/16 20:55:17, wtc wrote:
> We discussed this issue in person.
> 
> Your current solution is fine, but we should explain in this
> file what warning 4275 is (at least note it's a MSVC warning)
> and how one can fix the compiler warning.
> 
> A problem with including "base/compiler_specific.h" in this
> file is that this file itself doesn't use
> "base/compiler_specific.h".  I understand why you do it this
> way after looking at the other files, but I was wondering
> about it when I first saw it.

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_mapping_rules.h#n...
net/base/host_mapping_rules.h:18: class NET_TEST HostMappingRules {
On 2011/05/16 20:55:17, wtc wrote:
> I seem to remember there are command-line switches that use
> HostMappingRules.  So perhaps this should be NET_API?

That is performed with:
if (parsed_command_line.HasSwitch(switches::kHostRules))
    net::HttpStreamFactory::SetHostMappingRules(
        parsed_command_line.GetSwitchValueASCII(switches::kHostRules));

So HostMappingRules doesn't seem directly involved. Let me know if you still
want me to change this to be _API

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_port_pair.h#newco...
net/base/host_port_pair.h:18: class NET_TEST HostPortPair {
On 2011/05/16 20:55:17, wtc wrote:
> I suspect HostPortPair is used outside src/net, so it probably
> should be NET_API?

I missed that one. Thanks.

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_resolver_impl.h#n...
net/base/host_resolver_impl.h:63: class NET_API HostResolverImpl
On 2011/05/16 20:55:17, wtc wrote:
> Can this be NET_TEST?  "Impl" seems to imply it is an internal
> class.

Yup. It is used by four different files in chrome/content

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

http://codereview.chromium.org/6969077/diff/1/net/base/net_log.h#newcode284
net/base/net_log.h:284: class NET_TEST ScopedNetLogEvent {
On 2011/05/16 20:55:17, wtc wrote:
> This kind of scoped class seems useful, so probably should
> be NET_API?

The point is that nobody out of net uses this today. I just talked to Eric and
his opinion is to leave it as _TEST.

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

http://codereview.chromium.org/6969077/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:66: #if defined(UNIT_TEST) || defined(NET_DLL)
On 2011/05/16 20:55:17, wtc wrote:
> Perhaps we should just remove this ifdef?
> 
> Could you explain why NET_DLL also needs this method?

If the compiler, while building the dll doesn't "see" this line, it won't be
part of the dll, and the test won't be able to find it.

I removed the guards on all UNIT_TEST that are required in order to build.

[wtc, 2011-05-16 23:07:13]

LGTM.  Thanks!

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

http://codereview.chromium.org/6969077/diff/1/net/base/host_mapping_rules.h#n...
net/base/host_mapping_rules.h:18: class NET_TEST HostMappingRules {
Thank you for looking into this.  Please keep this as NET_TEST.

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

http://codereview.chromium.org/6969077/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:67: static void set_merge_chunks(bool merge) {
merge_chunks_ = merge; }
If appropriate, add a comment to say this method is intended
for unit tests only.

http://codereview.chromium.org/6969077/diff/9001/base/compiler_specific.h
File base/compiler_specific.h (right):

http://codereview.chromium.org/6969077/diff/9001/base/compiler_specific.h#new...
base/compiler_specific.h:57: // Allows exporting a class that inherits from a
non exported base class.
Nit: add '-' between 'non' and 'exported'.

http://codereview.chromium.org/6969077/diff/9001/base/compiler_specific.h#new...
base/compiler_specific.h:64: // Compiler warning C4275:
Nit: add "MSVC"

[rvargas (doing something else), 2011-05-17 21:11:06]

Brett, could you look at the files from base?

[brettw, 2011-05-18 06:06:18]

Base LGTM

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h
File base/compiler_specific.h (right):

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h#ne...
base/compiler_specific.h:62: // Class EXPORT_API Foo : NON_EXPORTED_BASE(public
Bar) {
Did you mean "Class" to be lower-case?

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h#ne...
base/compiler_specific.h:66: //
http://msdn.microsoft.com/en-us/library/3c594ae3(VS.80).aspx
Your URL doesn't seem to talk about C4275. I searched and found this page, is it
the right one?
http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx

It might also be good to mention that you need to be very careful if you have
static data in the base class, and to see the article for more.

[rvargas (doing something else), 2011-05-18 18:13:48]

Thanks.

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h
File base/compiler_specific.h (right):

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h#ne...
base/compiler_specific.h:62: // Class EXPORT_API Foo : NON_EXPORTED_BASE(public
Bar) {
On 2011/05/18 06:06:18, brettw wrote:
> Did you mean "Class" to be lower-case?

Yes, fixed.

http://codereview.chromium.org/6969077/diff/13012/base/compiler_specific.h#ne...
base/compiler_specific.h:66: //
http://msdn.microsoft.com/en-us/library/3c594ae3(VS.80).aspx
On 2011/05/18 06:06:18, brettw wrote:
> Your URL doesn't seem to talk about C4275. I searched and found this page, is
it
> the right one?
> http://msdn.microsoft.com/en-us/library/3tdb471s%28VS.80%29.aspx
> 
> It might also be good to mention that you need to be very careful if you have
> static data in the base class, and to see the article for more.

I added this to the comment:
// Note that this is intended to be used only when no access to the base class
// can be gained through the derived class. For more info, see
// http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx