NetLogEventParameter to Callback refactoring 8.

NetLogEventParameter to Callback refactoring 8.
Get rid of all uses of NetLogEventParameters in:

net/socket_stream, net/websockets, net/base.

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

[mmenke, 2012-06-14 14:18:02]

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:404: handshake_request_->GetRawRequest();
This mutates the handshake_request and thus is non-const, weirdly, so not
wrapping it in a callback.

[eroman, 2012-06-14 17:19:40]

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:408:
base::Bind(&NetLogWebSocketHandshakeCallback, &handshake_request));
This is confusing. There are two identically named functions, one in the header
takes a string, and one in this file which takes a
WebSocketHandshakeResponseHandler*.

Perhaps the one in this file should be deleted and all just use string version.

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:449: std::string raw_response =
handshake_response_->GetRawResponse();
This is unused. See comment above, perhaps you meant to change the line below to
pass &raw_response rather than handshake_response_.get()

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_n...
File net/websockets/websocket_net_log_params.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_n...
net/websockets/websocket_net_log_params.cc:12: Value*
NetLogWebSocketHandshakeCallback(const std::string* headers,
side-comment (no action necessary): I wonder why this is transforming the
input... I think in general it is better to pass the data down untouched for
debugging purposes.

[mmenke, 2012-06-14 17:30:06]

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:408:
base::Bind(&NetLogWebSocketHandshakeCallback, &handshake_request));
On 2012/06/14 17:19:40, eroman wrote:
> This is confusing. There are two identically named functions, one in the
header
> takes a string, and one in this file which takes a
> WebSocketHandshakeResponseHandler*.
> 
> Perhaps the one in this file should be deleted and all just use string
version.

They actually have different, but confusingly similar, names (Bind doesn't work
with overloaded functions, at least on Windows).

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:449: std::string raw_response =
handshake_response_->GetRawResponse();
On 2012/06/14 17:19:40, eroman wrote:
> This is unused. See comment above, perhaps you meant to change the line below
to
> pass &raw_response rather than handshake_response_.get()

Done.  That's what I did initially, actually but thought you'd prefer this. 
Just forgot to remove this line.

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_n...
File net/websockets/websocket_net_log_params.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_n...
net/websockets/websocket_net_log_params.cc:12: Value*
NetLogWebSocketHandshakeCallback(const std::string* headers,
On 2012/06/14 17:19:40, eroman wrote:
> side-comment (no action necessary): I wonder why this is transforming the
> input... I think in general it is better to pass the data down untouched for
> debugging purposes.

Does seem like a lot of work for an unclear purpose.

WebSockets are being re-implemented, anyways, so suspect this code will be gone
within a quarter or two.

[eroman, 2012-06-14 18:12:20]

lgtm

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
net/websockets/websocket_job.cc:449: std::string raw_response =
handshake_response_->GetRawResponse();
On 2012/06/14 17:30:06, Matt Menke wrote:
> On 2012/06/14 17:19:40, eroman wrote:
> > This is unused. See comment above, perhaps you meant to change the line
below
> to
> > pass &raw_response rather than handshake_response_.get()
> 
> Done.  That's what I did initially, actually but thought you'd prefer this. 
> Just forgot to remove this line.

I generally like deferring construction of the string, but like you pointed out
since GetRawResponse() is non-const, best not to gamble with fate :)

[mmenke, 2012-06-14 18:15:11]

On 2012/06/14 18:12:20, eroman wrote:
> lgtm
> 
>
http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
> File net/websockets/websocket_job.cc (right):
> 
>
http://codereview.chromium.org/10548028/diff/11004/net/websockets/websocket_j...
> net/websockets/websocket_job.cc:449: std::string raw_response =
> handshake_response_->GetRawResponse();
> On 2012/06/14 17:30:06, Matt Menke wrote:
> > On 2012/06/14 17:19:40, eroman wrote:
> > > This is unused. See comment above, perhaps you meant to change the line
> below
> > to
> > > pass &raw_response rather than handshake_response_.get()
> > 
> > Done.  That's what I did initially, actually but thought you'd prefer this. 
> > Just forgot to remove this line.
> 
> I generally like deferring construction of the string, but like you pointed
out
> since GetRawResponse() is non-const, best not to gamble with fate :)

Ahh...But WebSocketHandshakeResponseHandler's function is const.  It's
WebSocketHandshakeRequestHandler that isn't (Notice the request/response). 
Confusing enough that I think you're right about treating them both the same,
though.

[commit-bot: I haz the power, 2012-06-14 18:17:07]

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

[eroman, 2012-06-14 18:21:40]

My head is spinning... but I stand by my earlier LGTM.

[commit-bot: I haz the power, 2012-06-14 19:45:25]

Change committed as 142202