Decouple WebSocketChannelClient from WebSocketImpl

Decouple WebSocketChannelClient from WebSocketImpl

Ideally we want to simply make WebSocketImpl inherit from WebSocketChannelClient,
but we cannot do that because WebSocketChannelClient needs to be on Oilpan's heap
whereas WebSocketImpl cannot be on Oilpan's heap. Thus this CL introduces a proxy
class to decouple WebSocketImpl from WebSocketChannelClient.

BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179460

[haraken, 2014-08-01 20:26:39]

PTAL

[yhirano, 2014-08-04 01:05:53]

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:36: #include
"public/web/WebSocketClient.h"
No need to include WebSocketClient.h

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:38: #include "wtf/OwnPtr.h"
PassOwnPtr.h instead of OwnPtr.h

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:39: #include "wtf/RefPtr.h"
No need to include RefPtr.h

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:47: // Ideally we want to simply make
WebSocketImpl inherit from WebSocketChannelClient,
Please wrap comments in 80 columns.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:85: m_impl->didClose(status, code,
reason);
Clearing m_impl would be good.

WebSocketImpl* impl = m_impl;
m_impl = nullptr;
impl->didClose(status, code, reason);

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.cpp
File Source/web/WebSocketImpl.cpp (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.cpp...
Source/web/WebSocketImpl.cpp:71: #if !ENABLE(OILPAN)
WebSocketImpl is not on heap any more, so we can remove this macro.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.h
File Source/web/WebSocketImpl.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.h#n...
Source/web/WebSocketImpl.h:47: class WebURL;
+class WebSocketChannel;

https://codereview.chromium.org/419973007/diff/1/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/web.gypi#newcode234
Source/web/web.gypi:234: 'WebSocketChannelClient.h',
WebSocketChannelClientProxy.h?

[haraken, 2014-08-04 01:21:57]

Thanks for review.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:36: #include
"public/web/WebSocketClient.h"
On 2014/08/04 01:05:53, yhirano wrote:
> No need to include WebSocketClient.h

We need to include WebSocketClient.h for ClosingHandshakeCompletionStatus.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:38: #include "wtf/OwnPtr.h"
On 2014/08/04 01:05:53, yhirano wrote:
> PassOwnPtr.h instead of OwnPtr.h

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:39: #include "wtf/RefPtr.h"
On 2014/08/04 01:05:52, yhirano wrote:
> No need to include RefPtr.h

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:47: // Ideally we want to simply make
WebSocketImpl inherit from WebSocketChannelClient,
On 2014/08/04 01:05:52, yhirano wrote:
> Please wrap comments in 80 columns.

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:85: m_impl->didClose(status, code,
reason);
On 2014/08/04 01:05:52, yhirano wrote:
> Clearing m_impl would be good.
> 
> WebSocketImpl* impl = m_impl;
> m_impl = nullptr;
> impl->didClose(status, code, reason);
> 

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.cpp
File Source/web/WebSocketImpl.cpp (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.cpp...
Source/web/WebSocketImpl.cpp:71: #if !ENABLE(OILPAN)
On 2014/08/04 01:05:53, yhirano wrote:
> WebSocketImpl is not on heap any more, so we can remove this macro.

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.h
File Source/web/WebSocketImpl.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketImpl.h#n...
Source/web/WebSocketImpl.h:47: class WebURL;
On 2014/08/04 01:05:53, yhirano wrote:
> +class WebSocketChannel;

Done.

https://codereview.chromium.org/419973007/diff/1/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/web.gypi#newcode234
Source/web/web.gypi:234: 'WebSocketChannelClient.h',
On 2014/08/04 01:05:53, yhirano wrote:
> WebSocketChannelClientProxy.h?

Done.

[yhirano, 2014-08-04 01:48:28]

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:36: #include
"public/web/WebSocketClient.h"
On 2014/08/04 01:21:57, haraken wrote:
> On 2014/08/04 01:05:53, yhirano wrote:
> > No need to include WebSocketClient.h
> 
> We need to include WebSocketClient.h for ClosingHandshakeCompletionStatus.

ClosingHandshakeCompletionStatus used in this header is one defined in
WebSocketChannelClient, isn't it?

[haraken, 2014-08-04 02:34:17]

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/1/Source/web/WebSocketChannelC...
Source/web/WebSocketChannelClientProxy.h:36: #include
"public/web/WebSocketClient.h"
On 2014/08/04 01:48:28, yhirano wrote:
> On 2014/08/04 01:21:57, haraken wrote:
> > On 2014/08/04 01:05:53, yhirano wrote:
> > > No need to include WebSocketClient.h
> > 
> > We need to include WebSocketClient.h for ClosingHandshakeCompletionStatus.
> 
> ClosingHandshakeCompletionStatus used in this header is one defined in
> WebSocketChannelClient, isn't it?

You're right. Removed WebSocketClient.h from includes.

[yhirano, 2014-08-04 03:13:01]

lgtm

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
Source/web/WebSocketChannelClientProxy.h:2: * Copyright (C) 2011, 2012 Google
Inc.  All rights reserved.
Please use the new license.

[tyoshino (SeeGerritForStatus), 2014-08-04 03:36:07]

lgtm

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
Source/web/WebSocketChannelClientProxy.h:41: namespace blink { class
WebSocketChannel; }
remove

[tkent, 2014-08-04 04:27:37]

lgtm

[haraken, 2014-08-04 05:54:34]

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
File Source/web/WebSocketChannelClientProxy.h (right):

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
Source/web/WebSocketChannelClientProxy.h:2: * Copyright (C) 2011, 2012 Google
Inc.  All rights reserved.
On 2014/08/04 03:13:00, yhirano wrote:
> Please use the new license.

Done.

https://codereview.chromium.org/419973007/diff/40001/Source/web/WebSocketChan...
Source/web/WebSocketChannelClientProxy.h:41: namespace blink { class
WebSocketChannel; }
On 2014/08/04 03:36:07, tyoshino wrote:
> remove

Done.

[haraken, 2014-08-04 05:58:23]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-04 05:58:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/419973007/60001

[commit-bot: I haz the power, 2014-08-04 06:53:34]

Change committed as 179460