NetLogEventParameter to Callback refactoring 9.

NetLogEventParameter to Callback refactoring 9.
Get rid of all uses of NetLogEventParameters in
net/socket.

R=eroman@chromium.org
BUG=126243
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142224

[mmenke, 2012-06-14 14:35:07]

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
File net/socket/socket_net_log_params.h (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
net/socket/socket_net_log_params.h:10: #include "net/base/net_util.h"
This is the simplest way to get sockaddr and socklen_t.  Otherwise, we'd need
some platform-dependent headers.

[eroman, 2012-06-14 17:45:50]

http://codereview.chromium.org/10546162/diff/13005/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/nss_ssl_util.cc...
net/socket/nss_ssl_util.cc:236: dict->SetInteger("ssl_lib_error",
PR_GetError());
Please take PR_GetError() as a parameter.

My concern with deferring the PR_GetError() is that there is potentially a bunch
of code that runs before this is reached (i.e. other NetLog observers).

If any of that code were to have sideffects that changed the last PR_GetError(),
we could get an unexpected result here! Probably won't happen, but no need to
take that risk.

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
File net/socket/socket_net_log_params.h (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
net/socket/socket_net_log_params.h:10: #include "net/base/net_util.h"
On 2012/06/14 14:35:08, Matt Menke wrote:
> This is the simplest way to get sockaddr and socklen_t.  Otherwise, we'd need
> some platform-dependent headers.

How about:
#include "base/sys_addrinfo.h"

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:388: NetLog::EventType event_type) {
Might fit on one line

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1362: NetLog::IntegerCallback("cert_count",
0)));
[optional]: "cert_count" is repeated several times. Might consider extracting
this snippet of code to a helper. (the whole PostOrRunCallback swathe of code).

[mmenke, 2012-06-14 18:03:45]

thanks for the feedback.

http://codereview.chromium.org/10546162/diff/13005/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/nss_ssl_util.cc...
net/socket/nss_ssl_util.cc:236: dict->SetInteger("ssl_lib_error",
PR_GetError());
On 2012/06/14 17:45:50, eroman wrote:
> Please take PR_GetError() as a parameter.
> 
> My concern with deferring the PR_GetError() is that there is potentially a
bunch
> of code that runs before this is reached (i.e. other NetLog observers).
> 
> If any of that code were to have sideffects that changed the last
PR_GetError(),
> we could get an unexpected result here! Probably won't happen, but no need to
> take that risk.

Done.

I did think about that.  Then I thought that any observer using the network
stack would probably lock while writing to the NetLog, anyways.

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
File net/socket/socket_net_log_params.h (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
net/socket/socket_net_log_params.h:10: #include "net/base/net_util.h"
On 2012/06/14 17:45:50, eroman wrote:
> On 2012/06/14 14:35:08, Matt Menke wrote:
> > This is the simplest way to get sockaddr and socklen_t.  Otherwise, we'd
need
> > some platform-dependent headers.
> 
> How about:
> #include "base/sys_addrinfo.h"

Done (With the corrected path)

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:388: NetLog::EventType event_type) {
On 2012/06/14 17:45:50, eroman wrote:
> Might fit on one line

Done.

http://codereview.chromium.org/10546162/diff/13005/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1362: NetLog::IntegerCallback("cert_count",
0)));
On 2012/06/14 17:45:50, eroman wrote:
> [optional]: "cert_count" is repeated several times. Might consider extracting
> this snippet of code to a helper. (the whole PostOrRunCallback swathe of
code).

Done.

[eroman, 2012-06-14 18:08:34]

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
File net/socket/socket_net_log_params.h (right):

http://codereview.chromium.org/10546162/diff/13005/net/socket/socket_net_log_...
net/socket/socket_net_log_params.h:10: #include "net/base/net_util.h"
On 2012/06/14 18:03:45, Matt Menke wrote:
> On 2012/06/14 17:45:50, eroman wrote:
> > On 2012/06/14 14:35:08, Matt Menke wrote:
> > > This is the simplest way to get sockaddr and socklen_t.  Otherwise, we'd
> need
> > > some platform-dependent headers.
> > 
> > How about:
> > #include "base/sys_addrinfo.h"
> 
> Done (With the corrected path)

haha.... ooops.

http://codereview.chromium.org/10546162/diff/4015/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10546162/diff/4015/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:227: // Automatically looks up last PR error.
Remove.

[eroman, 2012-06-14 18:08:46]

lgtm

[mmenke, 2012-06-14 18:12:55]

http://codereview.chromium.org/10546162/diff/4015/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10546162/diff/4015/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:227: // Automatically looks up last PR error.
On 2012/06/14 18:08:34, eroman wrote:
> Remove.

Done.

[commit-bot: I haz the power, 2012-06-14 19:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10546162/10014

[commit-bot: I haz the power, 2012-06-14 19:34:53]

Failed to apply patch for net/socket/socket_net_log_params.cc:
While running patch -p0 --forward --force;
patching file net/socket/socket_net_log_params.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
net/socket/socket_net_log_params.cc.rej

[mmenke, 2012-06-14 19:36:04]

On 2012/06/14 19:34:53, I haz the power (commit-bot) wrote:
> Failed to apply patch for net/socket/socket_net_log_params.cc:
> While running patch -p0 --forward --force;
> patching file net/socket/socket_net_log_params.cc
> Hunk #1 FAILED at 2.
> 1 out of 1 hunk FAILED -- saving rejects to file
> net/socket/socket_net_log_params.cc.rej

Doesn't like the rename.  I'll land once I've run another Linux try job
(Failures there look unrelated, but I'm paranoid).