Use proper URLRequestContext for WebSocket.

Use proper URLRequestContext for WebSocket.

WebSocket is handled by SocketStreamHost and used URLRequestContext
created from Profile::GetDefaultRequestContext().
It should use URLRequestContext got from
ResourceDispatcherHost::Receiver, as
ResourceDispatcherHost::BeginRequest() does.

BUG=62152
TEST=none

[Randy Smith (Not in Mondays), 2010-11-30 18:17:47]

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
File chrome/browser/renderer_host/socket_stream_host.cc (right):

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
chrome/browser/renderer_host/socket_stream_host.cc:67: // so that we can use the
same request_id.
So the problem I have with this approach is that, to me, it violates the
GetRequestContext abstraction because we're basing how we call GetRequestContext
based on what we know of the implementations of it.  But that kind of judgment
is better made by someone who is well versed in how the code is used and what
abstractions actually matter to respect, and that's not me.  So I'm willing to
defer to Eric's opinion, and piggyback my LGTM (for this issue) on his.  Eric?

(If you have an alternative way to fix this, you might just want to postpone the
fix until you can fix it properly--an incognito mode violation isn't a *good*
thing, but is it extra-high priority to fix right now?  And if so, can that be
turned towards freeing up time to fix it in the alternative fashion?)

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
chrome/browser/renderer_host/socket_stream_host.cc:70: ChromeURLRequestContext*
context = static_cast<ChromeURLRequestContext*>(
Why make context of type ChromeURLRequestContext rather than URLRequestContext? 
It doesn't look like you use any methods on it, and that would get rid of a lot
of casts.  Am I missing something?

[eroman, 2010-12-03 06:16:39]

I have basically the same comments as Randy.

Please remove the casts of the URLRequestContext as it is unnecessary.

LGTM to checkin for fixing the bug.

But we need to clean-up the interaction between these classes as a follow-up at
some point, since relying on request_id of 0 to work is fragile and may well
break.

I am not an expert on this code so can't suggest better approaches.

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
File chrome/browser/renderer_host/socket_stream_host.cc (right):

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
chrome/browser/renderer_host/socket_stream_host.cc:64: //
ResourceMessageFilter::GetReuqestContext() doesn't check request_id
typo: RequestContext

[ukai, 2010-12-07 02:21:04]

I add darin as reviewer.

Darin, this is a fix that WebSocket uses proper URLRequestContext than default
request context.
I think we should use ResourceDispatcherHost::Receiver::GetRequestContext(), but
it requires request_id and ViewHostMsg_Resource_Request.
WebSocket (SocketStream) has different request_id and no
ViewHostMsg_Resource_Request.  This change just pass dummy data, since it looks
GetRequestContext() implementation just checks resource_type in
ViewHostMsg_Resource_Request.

I'm not sure we could change GetRequestContext() signature just passing resource
type, or we should refactor socket stream bridge merged into resource dispatcher
bridge so that we could use the same request_id.

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
File chrome/browser/renderer_host/socket_stream_host.cc (right):

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
chrome/browser/renderer_host/socket_stream_host.cc:64: //
ResourceMessageFilter::GetReuqestContext() doesn't check request_id
On 2010/12/03 06:16:39, eroman wrote:
> typo: RequestContext

Done.

http://codereview.chromium.org/5406002/diff/1/chrome/browser/renderer_host/so...
chrome/browser/renderer_host/socket_stream_host.cc:70: ChromeURLRequestContext*
context = static_cast<ChromeURLRequestContext*>(
On 2010/11/30 18:17:47, rdsmith wrote:
> Why make context of type ChromeURLRequestContext rather than
URLRequestContext? 
> It doesn't look like you use any methods on it, and that would get rid of a
lot
> of casts.  Am I missing something?

Done.

[ukai, 2010-12-13 10:48:29]

ping

[Randy Smith (Not in Mondays), 2010-12-13 16:45:41]

On 2010/12/13 10:48:29, ukai wrote:
> ping

My apologies, Ukai, I had mis-remembered that I had already given my LGTM.  LGTM
(with the caveats that I want to keep moving towards doing this "right").

[jam, 2010-12-15 01:07:30]

Hi, I actually would really prefer if this change is held off, while my
refactoring of ResourceDispatcherHost is being made (should be sent out
today or tomorrow).  I'm getting rid of Receiver*, which this change uses.
 I have a much easier way of getting the URLRequestContext.

Thanks!

On Mon, Dec 13, 2010 at 8:45 AM, <rdsmith@chromium.org> wrote:

> On 2010/12/13 10:48:29, ukai wrote:
>
>> ping
>>
>
> My apologies, Ukai, I had mis-remembered that I had already given my LGTM.
>  LGTM
> (with the caveats that I want to keep moving towards doing this "right").
>
>
>
> http://codereview.chromium.org/5406002/
>

[jam, 2010-12-16 00:58:28]

Thanks for waiting.  My change landed earlier today in
http://src.chromium.org/viewvc/chrome?view=rev&revision=69335

The SocketStreamDispatcherHost can just copy what ResourceMessageFilter
does, in having a setter for a URLRequestContextOverride object which it
keeps a reference to.  It can then call it whenever it needs the request.

On Tue, Dec 14, 2010 at 5:07 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> Hi, I actually would really prefer if this change is held off, while my
> refactoring of ResourceDispatcherHost is being made (should be sent out
> today or tomorrow).  I'm getting rid of Receiver*, which this change uses.
>  I have a much easier way of getting the URLRequestContext.
>
> Thanks!
>
>
> On Mon, Dec 13, 2010 at 8:45 AM, <rdsmith@chromium.org> wrote:
>
>> On 2010/12/13 10:48:29, ukai wrote:
>>
>>> ping
>>>
>>
>> My apologies, Ukai, I had mis-remembered that I had already given my LGTM.
>>  LGTM
>> (with the caveats that I want to keep moving towards doing this "right").
>>
>>
>>
>> http://codereview.chromium.org/5406002/
>>
>
>

[Randy Smith (Not in Mondays), 2010-12-26 19:44:28]

Noticed I hadn't published a nit going over my old issues.

http://codereview.chromium.org/5406002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/socket_stream_host.cc (right):

http://codereview.chromium.org/5406002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/socket_stream_host.cc:14: 
Nit: Remove extra blank line.