Clean-up inline members of nested classes (net/)

Clean-up inline members of nested classes (net/)

Due to a bug, the Clang-plugin style checker failed to warn about
inline constructors, destructors, non-empty virtual methods, etc.
for nested classes.

The plugin has been fixed, and this patch is part of a clean-up of all
the code that now causes the plugin to issue errors.

BUG=139346
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150917

[mmenke, 2012-08-09 15:13:35]

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
File net/base/network_change_notifier_mac.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
net/base/network_change_notifier_mac.cc:95: void
NetworkChangeNotifierMac::Forwarder::OnNetworkConfigChange(
nit:  put a black line between functions.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
File net/base/network_change_notifier_mac.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
net/base/network_change_notifier_mac.h:36: :
net_config_watcher_(net_config_watcher) {}
Shouldn't this be de-inlined as well?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
File net/disk_cache/rankings.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.cc:193: // XXX omitting the constructor name crashes
clang. reduce & file.
I can make no sense of this comment.

Also, should be in the form "TODO(hans): <blah>"

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.cc:198: : scoped_ptr<CacheRankingsBlock>(node),
rankings_(rankings) {}
nit:  Line breaks between functions.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/rankings.h
File net/disk_cache/rankings.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.h:72: rankings_->FreeRankingsBlock(get());
Shouldn't this also be de-inlined?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.cc:385: ConnectionTimeout() const {
nit:  I think this may be a little more readable:

HttpProxyClientSocketPool::HttpProxyConnectJobFactory::ConnectionTimeout(
    ) const {

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
File net/proxy/proxy_config_service_mac.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
net/proxy/proxy_config_service_mac.h:49: :
proxy_config_service_(proxy_config_service) {}
Shouldn't you de-inline this as well?  You did so with other initializer-only
constructors.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
net/proxy/proxy_config_service_mac.h:54: OVERRIDE;
nit:  OVERRIDE is generally not placed on its own line, so this should be:

    virtual void SetDynamicStoreNotificationKeys(
        SCDynamicStoreRef store) OVERRIDE;

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:1225: 
Remove extra line breaks.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_pool.h:260: virtual ~SSLConnectJobFactory() {}
Should empty virtual destructors be de-inlined as well?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
File net/socket/tcp_client_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_libevent.h:87: explicit
ReadWatcher(TCPClientSocketLibevent* socket) : socket_(socket) {}
Sholdn't this also be de-inlined?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_libevent.h:103: explicit
WriteWatcher(TCPClientSocketLibevent* socket) : socket_(socket) {}
Sholdn't this also be de-inlined?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
File net/socket_stream/socket_stream.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
net/socket_stream/socket_stream.cc:77:
SocketStream::RequestHeaders::~RequestHeaders() { data_ = NULL; }
This should be lower in the file, in the same order it's declared in - just
below ~SocketStream().

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
File net/socket_stream/socket_stream.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
net/socket_stream/socket_stream.h:194: RequestHeaders() : IOBuffer() {}
Should this be de-inlined?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
File net/tools/flip_server/balsa_headers.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.cc:84: }
nit:  This one should be declared after operator<<, since that's where it
appears in the header.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.cc:109: }
These should be down near the BalsaHeaders constructor, in the order they're
declared.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
File net/tools/flip_server/balsa_headers.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:424: }
Should these constructors be de-inlined?

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:447: iterator_base(it) { }
Another inline constructor.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:464: iterator_base(headers, index) {}
Another inline constructor.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:475: iterator_base(it.headers_, it.idx_)
{}
Two more inline constructors.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:489: iterator_base(headers, index) {}
Another inline constructor.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:507: }
Four more inline constructors.  Not going to bother commenting on the rest in
this file.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/udp/udp_socket_lib...
File net/udp/udp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/udp/udp_socket_lib...
net/udp/udp_socket_libevent.h:126: explicit ReadWatcher(UDPSocketLibevent*
socket) : socket_(socket) {}
Should these two be de-inlined?

[hans, 2012-08-09 15:35:42]

Thank you very much for the review! New patch uploaded.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
File net/base/network_change_notifier_mac.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
net/base/network_change_notifier_mac.cc:95: void
NetworkChangeNotifierMac::Forwarder::OnNetworkConfigChange(
On 2012/08/09 15:13:35, Matt Menke wrote:
> nit:  put a black line between functions.

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
File net/base/network_change_notifier_mac.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
net/base/network_change_notifier_mac.h:36: :
net_config_watcher_(net_config_watcher) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Shouldn't this be de-inlined as well?

The plugin doesn't consider it to be complex enough to require that. It's using
a points-based system:
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors...

I can out-of-line it for consistency if you'd like, but the plugin isn't
complaining about it.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
File net/disk_cache/rankings.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.cc:193: // XXX omitting the constructor name crashes
clang. reduce & file.
On 2012/08/09 15:13:35, Matt Menke wrote:
> I can make no sense of this comment.
> 
> Also, should be in the form "TODO(hans): <blah>"

I think this can just be removed.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.cc:198: : scoped_ptr<CacheRankingsBlock>(node),
rankings_(rankings) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> nit:  Line breaks between functions.

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/rankings.h
File net/disk_cache/rankings.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/disk_cache/ranking...
net/disk_cache/rankings.h:72: rankings_->FreeRankingsBlock(get());
On 2012/08/09 15:13:35, Matt Menke wrote:
> Shouldn't this also be de-inlined?

The plugin wasn't complaining about it. I can out-of-line it for consitency if
you'd like.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.cc:385: ConnectionTimeout() const {
On 2012/08/09 15:13:35, Matt Menke wrote:
> nit:  I think this may be a little more readable:
> 
> HttpProxyClientSocketPool::HttpProxyConnectJobFactory::ConnectionTimeout(
>     ) const {

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
File net/proxy/proxy_config_service_mac.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
net/proxy/proxy_config_service_mac.h:49: :
proxy_config_service_(proxy_config_service) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Shouldn't you de-inline this as well?  You did so with other initializer-only
> constructors.

Those constructors were considered more complex by the plugin. I can out-of-line
this for consistency if you'd like.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/proxy/proxy_config...
net/proxy/proxy_config_service_mac.h:54: OVERRIDE;
On 2012/08/09 15:13:35, Matt Menke wrote:
> nit:  OVERRIDE is generally not placed on its own line, so this should be:
> 
>     virtual void SetDynamicStoreNotificationKeys(
>         SCDynamicStoreRef store) OVERRIDE;

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:1225: 
On 2012/08/09 15:13:35, Matt Menke wrote:
> Remove extra line breaks.

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_pool.h:260: virtual ~SSLConnectJobFactory() {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Should empty virtual destructors be de-inlined as well?

The plugin didn't complain, so this is fine.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
File net/socket/tcp_client_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_libevent.h:87: explicit
ReadWatcher(TCPClientSocketLibevent* socket) : socket_(socket) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Sholdn't this also be de-inlined?

The plugin didn't complain about it.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_libevent.h:103: explicit
WriteWatcher(TCPClientSocketLibevent* socket) : socket_(socket) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Sholdn't this also be de-inlined?

The plugin didn't complain about it.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
File net/socket_stream/socket_stream.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
net/socket_stream/socket_stream.cc:77:
SocketStream::RequestHeaders::~RequestHeaders() { data_ = NULL; }
On 2012/08/09 15:13:35, Matt Menke wrote:
> This should be lower in the file, in the same order it's declared in - just
> below ~SocketStream().

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
File net/socket_stream/socket_stream.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/socket_stream/sock...
net/socket_stream/socket_stream.h:194: RequestHeaders() : IOBuffer() {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Should this be de-inlined?

The plugin didn't complain about it.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
File net/tools/flip_server/balsa_headers.cc (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.cc:84: }
On 2012/08/09 15:13:35, Matt Menke wrote:
> nit:  This one should be declared after operator<<, since that's where it
> appears in the header.

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.cc:109: }
On 2012/08/09 15:13:35, Matt Menke wrote:
> These should be down near the BalsaHeaders constructor, in the order they're
> declared.

Done.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
File net/tools/flip_server/balsa_headers.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:424: }
On 2012/08/09 15:13:35, Matt Menke wrote:
> Should these constructors be de-inlined?

The plugin doesn't think it's complex enough.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:447: iterator_base(it) { }
On 2012/08/09 15:13:35, Matt Menke wrote:
> Another inline constructor.

Ditto.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:464: iterator_base(headers, index) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Another inline constructor.

Ditto.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:475: iterator_base(it.headers_, it.idx_)
{}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Two more inline constructors.

Ditto.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:489: iterator_base(headers, index) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Another inline constructor.

Ditto.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/tools/flip_server/...
net/tools/flip_server/balsa_headers.h:507: }
On 2012/08/09 15:13:35, Matt Menke wrote:
> Four more inline constructors.  Not going to bother commenting on the rest in
> this file.

Ditto.

https://chromiumcodereview.appspot.com/10854063/diff/1/net/udp/udp_socket_lib...
File net/udp/udp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/udp/udp_socket_lib...
net/udp/udp_socket_libevent.h:126: explicit ReadWatcher(UDPSocketLibevent*
socket) : socket_(socket) {}
On 2012/08/09 15:13:35, Matt Menke wrote:
> Should these two be de-inlined?

The plugin doesn't complain about it.

[mmenke, 2012-08-09 15:41:50]

LGTM

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
File net/base/network_change_notifier_mac.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/1/net/base/network_chang...
net/base/network_change_notifier_mac.h:36: :
net_config_watcher_(net_config_watcher) {}
On 2012/08/09 15:35:42, hans wrote:
> On 2012/08/09 15:13:35, Matt Menke wrote:
> > Shouldn't this be de-inlined as well?
> 
> The plugin doesn't consider it to be complex enough to require that. It's
using
> a points-based system:
>
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors...
> 
> I can out-of-line it for consistency if you'd like, but the plugin isn't
> complaining about it.

I'm fine with it as-is, just wasn't sure if you were missing stuff or not. 
Thanks for the link.

[commit-bot: I haz the power, 2012-08-09 15:42:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/10854063/7002

[commit-bot: I haz the power, 2012-08-09 16:45:48]

Try job failure for 10854063-7002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[mmenke, 2012-08-09 17:05:09]

https://chromiumcodereview.appspot.com/10854063/diff/7002/net/base/transport_...
File net/base/transport_security_state.h (right):

https://chromiumcodereview.appspot.com/10854063/diff/7002/net/base/transport_...
net/base/transport_security_state.h:154: class Iterator {
Looks like this should be NET_EXPORT.

[commit-bot: I haz the power, 2012-08-09 18:15:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/10854063/4010

[commit-bot: I haz the power, 2012-08-09 19:09:18]

Try job failure for 10854063-4010 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-09 19:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/10854063/13003

[commit-bot: I haz the power, 2012-08-09 20:21:54]

Try job failure for 10854063-13003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-09 20:42:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/10854063/13003

[mmenke, 2012-08-09 20:44:22]

On 2012/08/09 20:42:35, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/hans%40chromium.org/10854063/13003

You can just go ahead and commit if you like, though CQing again is fine, too -
that failing test was mine, and was just re-enabled after a fix for flakiness
that didn't take.

[commit-bot: I haz the power, 2012-08-09 22:27:32]

Change committed as 150917