Bind RenderViewImpl routing_id to SocketStreamHost

Bind RenderViewImpl routing_id to SocketStreamHost

 - Set RenderViewImpl's routing_id to WebSocketStresmHandleImpl via
  dispatchWillOpenSocketStream()
 - Send the id from WebSocketStreamHandleImpl to SocketStreamHost
  via IPC SocketStreamHostMsg_Connect

This change aim that SocketStreamDispatcher uses SSLManager
with routing_id to handle SSL errors.

BUG=53836
TEST=none

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

[Takashi Toyoshima, 2012-03-13 20:06:51]

Hi,
This is a part of my change to fix issue 53836.

I feel Darin is the best person to review because Darin know
WebKit side change and OWNERS in major directories.

Also, I'd like to add Wan-Teh because Wan-Teh looks around
my CLs to fix this issue 53836 and also familier with network stack.

Actually, this change doesn't work without following WebKit change.
https://bugs.webkit.org/show_bug.cgi?id=78459
But, doesn't depend on the change to build.
I think this change is useful to understand how I'll handle
routing_id with WebKit side change.

[jam, 2012-03-15 02:01:19]

I looked at this when looking at 9704045. mostly good with just some nits

http://codereview.chromium.org/9677031/diff/5001/content/common/socket_stream...
File content/common/socket_stream_messages.h (right):

http://codereview.chromium.org/9677031/diff/5001/content/common/socket_stream...
content/common/socket_stream_messages.h:27: int /* routing_id */,
nit: might want to call this render_view_id to be clearer

http://codereview.chromium.org/9677031/diff/5001/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9677031/diff/5001/content/renderer/render_view...
content/renderer/render_view_impl.h:578: 
no need for blank line here (and while you're at it, remove ones above)

http://codereview.chromium.org/9677031/diff/5001/webkit/glue/websocketstreamh...
File webkit/glue/websocketstreamhandle_impl.h (right):

http://codereview.chromium.org/9677031/diff/5001/webkit/glue/websocketstreamh...
webkit/glue/websocketstreamhandle_impl.h:28: // Handle RenderView routing id
associated with the WebSocket object.
nit: don't mention RenderView or routing_id's at the webkit layer, even in
comments

[Takashi Toyoshima, 2012-03-15 22:49:03]

http://codereview.chromium.org/9677031/diff/5001/content/common/socket_stream...
File content/common/socket_stream_messages.h (right):

http://codereview.chromium.org/9677031/diff/5001/content/common/socket_stream...
content/common/socket_stream_messages.h:27: int /* routing_id */,
On 2012/03/15 02:01:19, John Abd-El-Malek wrote:
> nit: might want to call this render_view_id to be clearer

Done.

http://codereview.chromium.org/9677031/diff/5001/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9677031/diff/5001/content/renderer/render_view...
content/renderer/render_view_impl.h:578: 
I see.
I removed blank lines around here.

http://codereview.chromium.org/9677031/diff/5001/webkit/glue/websocketstreamh...
File webkit/glue/websocketstreamhandle_impl.h (right):

http://codereview.chromium.org/9677031/diff/5001/webkit/glue/websocketstreamh...
webkit/glue/websocketstreamhandle_impl.h:28: // Handle RenderView routing id
associated with the WebSocket object.
Should I abstract them?
In the case of WebKit side change, Darin suggests ExtraData approach. I try that
approach here, too.

[jam, 2012-03-16 07:33:59]

lgtm

you can also derive from base::SupportsUserData

http://codereview.chromium.org/9677031/diff/5005/content/renderer/websocketst...
File content/renderer/websocketstreamhandle_impl_extradata_impl.h (right):

http://codereview.chromium.org/9677031/diff/5005/content/renderer/websocketst...
content/renderer/websocketstreamhandle_impl_extradata_impl.h:21: int
RenderViewId() const { return render_view_id_; }
nit: google style is render_view_id() for inline getters

[Takashi Toyoshima, 2012-03-16 21:44:18]

Thank you John.

> you can also derive from base::SupportsUserData

Do you mean I can make dependency on base/* from webkit/glue?
Sorry for my poor understanding on API layering.

[Takashi Toyoshima, 2012-03-16 22:09:26]

http://codereview.chromium.org/9677031/diff/5005/content/renderer/websocketst...
File content/renderer/websocketstreamhandle_impl_extradata_impl.h (right):

http://codereview.chromium.org/9677031/diff/5005/content/renderer/websocketst...
content/renderer/websocketstreamhandle_impl_extradata_impl.h:21: int
RenderViewId() const { return render_view_id_; }
On 2012/03/16 07:33:59, John Abd-El-Malek wrote:
> nit: google style is render_view_id() for inline getters

Done.

[jam, 2012-03-19 16:00:47]

On 2012/03/16 21:44:18, toyoshim wrote:
> Thank you John.
> 
> > you can also derive from base::SupportsUserData
> 
> Do you mean I can make dependency on base/* from webkit/glue?

yes, there are already other includes from there

> Sorry for my poor understanding on API layering.

[Takashi Toyoshima, 2012-03-19 20:43:09]

Hi, jamesr@ and tony@.

This CL requires one more OWNERS review on webkit/.
So, I added jamesr@ and tony@.

Actually, this CL have no mean without WebKit side CL.
So, if I got webkit/OWNERS LGTM, I'll postpone submitting until WebKit side CL
get r+.

FYI; https://bugs.webkit.org/show_bug.cgi?id=78459

[tony, 2012-03-19 20:52:56]

webkit/glue LGTM

https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websock...
File webkit/glue/websocketstreamhandle_impl.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websock...
webkit/glue/websocketstreamhandle_impl.h:30: virtual void close() OVERRIDE;
I would not mark these as OVERRIDE.  It makes refactoring harder since the
interface lives in the upstream repository.

[darin (slow to review), 2012-03-19 20:58:28]

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/sock...
File content/common/socket_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/sock...
content/common/socket_stream_dispatcher.cc:152:
webkit_glue::WebSocketStreamHandleImpl* handle =
you might consider hiding some of this goop behind a static method on
SocketHandleData:

  SocketHandleData* data = SocketHandleData::ForHandle(handle_);
  if (data)
    render_view_id = data->render_view_id();

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/re...
content/renderer/render_view_impl.cc:3411: impl->setExtraData(
since this is all chromium-side, the more canonical solution is to leverage
base/supports_user_data.h

WebSocketStreamHandleImpl would extend from base::SupportsUserData in other
words.

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/we...
File content/renderer/websocketstreamhandle_impl_extradata_impl.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/we...
content/renderer/websocketstreamhandle_impl_extradata_impl.h:13: class
WebSocketStreamHandleImplExtraDataImpl
nit: use a shorter name here?

How about SocketHandleData?

[Takashi Toyoshima, 2012-03-19 22:58:44]

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/sock...
File content/common/socket_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/sock...
content/common/socket_stream_dispatcher.cc:152:
webkit_glue::WebSocketStreamHandleImpl* handle =
Is doing static cast in SocketHandleData::ForHandle() OK?
I feel it's a little dangerous and curious because caller should always assure
that the handle is webkit_glue::WebSocketStreamHandleImpl in spite of the
declaration type being WebKit::WebSocketStreamHandle.

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/re...
content/renderer/render_view_impl.cc:3411: impl->setExtraData(
On 2012/03/19 20:58:28, darin wrote:
> since this is all chromium-side, the more canonical solution is to leverage
> base/supports_user_data.h
> 
> WebSocketStreamHandleImpl would extend from base::SupportsUserData in other
> words.

Done.

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/we...
File content/renderer/websocketstreamhandle_impl_extradata_impl.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/we...
content/renderer/websocketstreamhandle_impl_extradata_impl.h:13: class
WebSocketStreamHandleImplExtraDataImpl
OK.
I know that *SocketHandle exist in net/socket. These are not WebSocket specific
classes but more generic one. So SocketHandleData could be confusing. How about
SocketStreamHandleData. It's longer than your proposal. But, SocketStream is a
good keyword for WebSocket.

BTW, should I move this file to content/common/?
content/common/socket_stream_dispatcher has a dependency on this file, and other
files in content/common looks having no dependency on content/renderer.

https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websock...
File webkit/glue/websocketstreamhandle_impl.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websock...
webkit/glue/websocketstreamhandle_impl.h:30: virtual void close() OVERRIDE;
On 2012/03/19 20:52:56, tony wrote:
> I would not mark these as OVERRIDE.  It makes refactoring harder since the
> interface lives in the upstream repository.

Done.

[darin (slow to review), 2012-03-19 23:06:26]

On Mon, Mar 19, 2012 at 3:58 PM, <toyoshim@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
>
16001/content/common/socket_**stream_dispatcher.cc<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/socket_stream_dispatcher.cc>
> File content/common/socket_stream_**dispatcher.cc (right):
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
>
16001/content/common/socket_**stream_dispatcher.cc#**newcode152<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/common/socket_stream_dispatcher.cc#newcode152>
> content/common/socket_stream_**dispatcher.cc:152:
> webkit_glue::**WebSocketStreamHandleImpl* handle =
> Is doing static cast in SocketHandleData::ForHandle() OK?
> I feel it's a little dangerous and curious because caller should always
> assure that the handle is webkit_glue::**WebSocketStreamHandleImpl in
> spite of the declaration type being WebKit::WebSocketStreamHandle.


I think that's OK assuming Chrome only ever has a single implementation
of WebSocketStreamHandle.  We make that kind of assumption in other
places too.  You just have to be confident that this will be the only impl.



>
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
>
16001/content/renderer/render_**view_impl.cc<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
>
16001/content/renderer/render_**view_impl.cc#newcode3411<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/render_view_impl.cc#newcode3411>
> content/renderer/render_view_**impl.cc:3411: impl->setExtraData(
> On 2012/03/19 20:58:28, darin wrote:
>
>> since this is all chromium-side, the more canonical solution is to
>>
> leverage
>
>> base/supports_user_data.h
>>
>
>  WebSocketStreamHandleImpl would extend from base::SupportsUserData in
>>
> other
>
>> words.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
>
16001/content/renderer/**websocketstreamhandle_impl_**extradata_impl.h<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/websocketstreamhandle_impl_extradata_impl.h>
> File content/renderer/**websocketstreamhandle_impl_**extradata_impl.h
> (right):
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**
> 16001/content/renderer/**websocketstreamhandle_impl_**
>
extradata_impl.h#newcode13<https://chromiumcodereview.appspot.com/9677031/diff/16001/content/renderer/websocketstreamhandle_impl_extradata_impl.h#newcode13>
> content/renderer/**websocketstreamhandle_impl_**extradata_impl.h:13: class
> WebSocketStreamHandleImplExtra**DataImpl
> OK.
> I know that *SocketHandle exist in net/socket. These are not WebSocket
> specific classes but more generic one. So SocketHandleData could be
> confusing. How about SocketStreamHandleData. It's longer than your
> proposal. But, SocketStream is a good keyword for WebSocket.
>

SocketStreamHandleData sounds fine to me.  By having the static ForHandle
function take a WebKit::WebSocketStreamHandle*, I was trying to avoid having
to have consumers see / play with WebSocketStreamHandleImpl.  Make sense?



>
> BTW, should I move this file to content/common/?
> content/common/socket_stream_**dispatcher has a dependency on this file,
> and other files in content/common looks having no dependency on
> content/renderer.
>
>
Yes, good catch.  content/common/ should not depend on content/renderer/.



>
> https://chromiumcodereview.**appspot.com/9677031/diff/**16001/webkit/glue/
>
**websocketstreamhandle_impl.h<https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websocketstreamhandle_impl.h>
> File webkit/glue/**websocketstreamhandle_impl.h (right):
>
> https://chromiumcodereview.**appspot.com/9677031/diff/**16001/webkit/glue/
>
**websocketstreamhandle_impl.h#**newcode30<https://chromiumcodereview.appspot.com/9677031/diff/16001/webkit/glue/websocketstreamhandle_impl.h#newcode30>
> webkit/glue/**websocketstreamhandle_impl.h:**30: virtual void close()
> OVERRIDE;
> On 2012/03/19 20:52:56, tony wrote:
>
>> I would not mark these as OVERRIDE.  It makes refactoring harder since
>>
> the
>
>> interface lives in the upstream repository.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/9677031/<https://chromiumcodereview....
>

[wtc, 2012-03-19 23:21:03]

Drive-by review comments on patch set 7:

toyoshim: several people more qualified than me already
reviewed this CL.  Please consider my review a drive-by
review.  I have some questions and minor suggestions.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/browser/ren...
File content/browser/renderer_host/socket_stream_host.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/browser/ren...
content/browser/renderer_host/socket_stream_host.h:32: int routing_id,

Why don't you call this render_view_id?  Is it because
there are other kinds of routing id besides render view id?

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_handle_data.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_handle_data.h:25: // Picks the stored user data
from WebKit::WebSocketStreamHandle object.

Nit: Picks => Retrieves
           or Extracts

Nit: your use of "Virtually" on the next line sounds a little
strange to me.  I think you meant "Actually".  I suggest
you say:
  |handle| must actually be a WebSocketStreamHandleImpl object.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_messages.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_messages.h:25: // WebSocket belongs.

Nit: should "WebSocket" be changed "Socket Stream"?

[Takashi Toyoshima, 2012-03-19 23:34:56]

> I think that's OK assuming Chrome only ever has a single implementation
> of WebSocketStreamHandle.  We make that kind of assumption in other
> places too.  You just have to be confident that this will be the only impl.

OK, I agreed. I applied this change at Patch Set 7.

> SocketStreamHandleData sounds fine to me.  By having the static ForHandle
> function take a WebKit::WebSocketStreamHandle*, I was trying to avoid having
> to have consumers see / play with WebSocketStreamHandleImpl.  Make sense?

I see. I missed removing header inclusion at Patch Set 7.
In Patch Set 8, SocketStreamDispatcher is totally free from understanding
WebSocketStreamHandleImpl.
But, why only for consumer side? How about adding static void
SetTo(WebKit::WebSocketStreamHandle*, SocketStreamHandleData*) and
SocketStreamHandleData* GetFrom(WebKit::WebSocketStreamHandle*) at
SocketStreamHandleData?

> Yes, good catch.  content/common/ should not depend on content/renderer/.

Thanks. I moved them into content/common and removed the dependency.

[Takashi Toyoshima, 2012-03-20 00:18:03]

Thank you all reviewers.
I believe I reflects all reviewers' comments at Patch Set 9.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/browser/ren...
File content/browser/renderer_host/socket_stream_host.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/browser/ren...
content/browser/renderer_host/socket_stream_host.h:32: int routing_id,
No. render_view_id is the only one as routing_id here.
I rename all routing_id in this CL.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_handle_data.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_handle_data.h:25: // Picks the stored user data
from WebKit::WebSocketStreamHandle object.
Thank you.
"Actually" must be the right word that means I want to say.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_messages.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_messages.h:25: // WebSocket belongs.
Thank you.
It looks better.
Done.

[darin (slow to review), 2012-03-20 06:42:37]

On Mon, Mar 19, 2012 at 4:34 PM, <toyoshim@chromium.org> wrote:

> I think that's OK assuming Chrome only ever has a single implementation
>> of WebSocketStreamHandle.  We make that kind of assumption in other
>> places too.  You just have to be confident that this will be the only
>> impl.
>>
>
> OK, I agreed. I applied this change at Patch Set 7.
>
>
>  SocketStreamHandleData sounds fine to me.  By having the static ForHandle
>> function take a WebKit::WebSocketStreamHandle***, I was trying to avoid
>> having
>> to have consumers see / play with WebSocketStreamHandleImpl.  Make sense?
>>
>
> I see. I missed removing header inclusion at Patch Set 7.
> In Patch Set 8, SocketStreamDispatcher is totally free from understanding
> WebSocketStreamHandleImpl.
> But, why only for consumer side? How about adding static void
> SetTo(WebKit::**WebSocketStreamHandle*, SocketStreamHandleData*) and
> SocketStreamHandleData* GetFrom(WebKit::**WebSocketStreamHandle*) at
> SocketStreamHandleData?


Good point!  You could perhaps just add a non-static member function for
associating a SocketStreamHandleData with a WebSocketStreamHandle.  Maybe
called AddToHandle?

I was thinking that it should be non-static since you'd first allocate a
SocketStreamHandleData and then assign it to a WebSocketStreamHandle.  So,
it could be a non-static function.  The lookup function needs to be static
though.



>
>
>  Yes, good catch.  content/common/ should not depend on content/renderer/.
>>
>
> Thanks. I moved them into content/common and removed the dependency.
>
>
>
https://chromiumcodereview.**appspot.com/9677031/<https://chromiumcodereview....
>

[darin (slow to review), 2012-03-20 06:46:59]

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_dispatcher.cc:21: #include
"webkit/glue/websocketstreamhandle_impl.h"
still needed?

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/common/sock...
File content/common/socket_stream_handle_data.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/common/sock...
content/common/socket_stream_handle_data.cc:17: if (!impl)
nit: impl will only be null if handle was null.  maybe it would be better to
null-check handle first before doing anything else?

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/renderer/re...
content/renderer/render_view_impl.cc:3408: WebSocketStreamHandleImpl* impl =
this could either be:

  SocketStreamHandleData::AddToHandle(handle, routing_id_);

or it could be:

  SocketStreamHandleData* data =
      new SocketStreamHandleData(routing_id_);
  data->AddToHandle(handle);

the first approach entirely hides the allocation of the SocketStreamHandleData. 
maybe that is a nice thing?  hmm....

[Takashi Toyoshima, 2012-03-20 18:19:39]

Thank you, Darin.
My CL looks pretty better than before.

If you grant me, I'll land this change because WebKit API revision is well fixed
now, and will be landed soon.

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
File content/common/socket_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/20002/content/common/sock...
content/common/socket_stream_dispatcher.cc:21: #include
"webkit/glue/websocketstreamhandle_impl.h"
No. This inclusion was removed at Patch Set 8.

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/common/sock...
File content/common/socket_stream_handle_data.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/common/sock...
content/common/socket_stream_handle_data.cc:17: if (!impl)
Agreed.
Fixed in the Patch Set 10.

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/18010/content/renderer/re...
content/renderer/render_view_impl.cc:3408: WebSocketStreamHandleImpl* impl =
I try the second approach at Patch Set 10.
And the first at Patch Set 11.

The first approarch, Static AddToHandle() hiding the allocation with routing_id,
looks smarter than the second one.

So, I'd like to apply the first one.

[darin (slow to review), 2012-03-20 20:01:00]

LGTM

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
File content/common/socket_stream_handle_data.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
content/common/socket_stream_handle_data.cc:15: DCHECK(handle);
nit: more conventional to use NOTREACHED here so you don't have to repeat the
conditional:

  if (!handle) {
    NOTREACHED();
    return;
  }

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
File content/common/socket_stream_handle_data.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
content/common/socket_stream_handle_data.h:18: explicit
SocketStreamHandleData(int render_view_id)
nit: you can now make the constructor private since AddToHandle should be the
only way to allocate one of these objects.

[commit-bot: I haz the power, 2012-03-21 00:57:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9677031/33001

[Takashi Toyoshima, 2012-03-21 00:57:17]

Thank you.
I applied these comments before submitting.

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
File content/common/socket_stream_handle_data.cc (right):

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
content/common/socket_stream_handle_data.cc:15: DCHECK(handle);
On 2012/03/20 20:01:00, darin wrote:
> nit: more conventional to use NOTREACHED here so you don't have to repeat the
> conditional:
> 
>   if (!handle) {
>     NOTREACHED();
>     return;
>   }

Done.

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
File content/common/socket_stream_handle_data.h (right):

https://chromiumcodereview.appspot.com/9677031/diff/29001/content/common/sock...
content/common/socket_stream_handle_data.h:18: explicit
SocketStreamHandleData(int render_view_id)
On 2012/03/20 20:01:00, darin wrote:
> nit: you can now make the constructor private since AddToHandle should be the
> only way to allocate one of these objects.

Done.

[commit-bot: I haz the power, 2012-03-21 02:26:22]

Try job failure for 9677031-33001 (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-03-21 05:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9677031/33001

[commit-bot: I haz the power, 2012-03-21 06:20:44]

Try job failure for 9677031-33001 (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...

[Takashi Toyoshima, 2012-03-21 13:07:43]

Oops, this looks actual compile error.
I guess ForHandle must be exported as CONTENT_EXPORT for shared built
unit_tests.
I git try -b win_rel,linux_shared to check this fixes the build issue, then will
push to CQ.

[commit-bot: I haz the power, 2012-03-21 14:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9677031/43001

[commit-bot: I haz the power, 2012-03-21 15:39:40]

Change committed as 127964