Move WebSocketHandleImpl into Blink (issue 2224713002 by darin@chromium.org)

3 views
Skip to first unread message

da...@chromium.org

unread,
Aug 22, 2016, 1:28:34 PM8/22/16
to yhi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org
Reviewers: yhirano
CL: https://codereview.chromium.org/2224713002/


https://codereview.chromium.org/2224713002/diff/180001/third_party/WebKit/Source/modules/modules.gypi
File third_party/WebKit/Source/modules/modules.gypi (right):

https://codereview.chromium.org/2224713002/diff/180001/third_party/WebKit/Source/modules/modules.gypi#newcode2096
third_party/WebKit/Source/modules/modules.gypi:2096:
'websockets/WebSocketHandle.cpp',
This is here because DocumentWebSocketChannelTest now has a dependency
on WebSocketHandle. That used to be an interface w/ just a bunch of pure
virtual methods. Now, instead, it is the real concrete class with
virtual methods that can be overridden for the sake of testing. I did
that because it seemed better to have less code (i.e., no
WebSocketHandleImpl class), but it seems like an uncommon approach, so
flagging here for consideration.

Description:
Move WebSocketHandleImpl into Blink

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+407, -1108 lines):
M content/browser/DEPS
M content/browser/websockets/websocket_impl.h
M content/browser/websockets/websocket_impl.cc
M content/browser/websockets/websocket_manager.h
M content/browser/websockets/websocket_manager.cc
M content/browser/websockets/websocket_manager_unittest.cc
M content/common/BUILD.gn
D content/common/websocket.mojom
M content/content_common_mojo_bindings.gyp
M content/content_renderer.gypi
M content/renderer/render_frame_impl.h
M content/renderer/render_frame_impl.cc
M content/renderer/renderer_blink_platform_impl.h
M content/renderer/renderer_blink_platform_impl.cc
D content/renderer/websockethandle_impl.h
D content/renderer/websockethandle_impl.cc
M third_party/WebKit/Source/core/loader/FrameLoaderClient.h
M third_party/WebKit/Source/modules/modules.gypi
M third_party/WebKit/Source/modules/websockets/DEPS
M third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h
M third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
M third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannelTest.cpp
A + third_party/WebKit/Source/modules/websockets/WebSocketHandle.h
A third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp
A + third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
M third_party/WebKit/Source/platform/blink_platform.gypi
D third_party/WebKit/Source/platform/exported/WebSocketHandshakeRequestInfo.cpp
D third_party/WebKit/Source/platform/exported/WebSocketHandshakeResponseInfo.cpp
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
M third_party/WebKit/Source/web/WebSharedWorkerImpl.h
M third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
M third_party/WebKit/public/BUILD.gn
M third_party/WebKit/public/blink_headers.gypi
M third_party/WebKit/public/platform/Platform.h
M third_party/WebKit/public/platform/modules/websockets/OWNERS
D third_party/WebKit/public/platform/modules/websockets/WebSocketHandle.h
D third_party/WebKit/public/platform/modules/websockets/WebSocketHandleClient.h
D third_party/WebKit/public/platform/modules/websockets/WebSocketHandshakeRequestInfo.h
D third_party/WebKit/public/platform/modules/websockets/WebSocketHandshakeResponseInfo.h
A + third_party/WebKit/public/platform/modules/websockets/websocket.mojom
M third_party/WebKit/public/web/WebFrameClient.h


yhi...@chromium.org

unread,
Aug 24, 2016, 1:01:01 AM8/24/16
to da...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp#newcode15
third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp:15:
#include "wtf/Functional.h"
+wtf/text/WTFString.h

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.h
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.h#newcode36
third_party/WebKit/Source/modules/websockets/WebSocketHandle.h:36:
#include "wtf/Vector.h"
+wtf/Forward.h

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
File
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(left):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h#oldcode63
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h:63:
virtual void didFail(WebSocketHandle* /* handle */, const WebString&
message) = 0;
Can you leave a commented out parameter name if it is used in the
function comment? Ditto below.

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
File
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h#newcode34
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h:34:
#include "modules/websockets/WebSocketHandle.h"
+wtf/Forward.h

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/public/platform/modules/websockets/OWNERS
File third_party/WebKit/public/platform/modules/websockets/OWNERS
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/public/platform/modules/websockets/OWNERS#newcode2
third_party/WebKit/public/platform/modules/websockets/OWNERS:2:
yhi...@chromium.org
I originally created this directory in order to give us ownership of the
related files. Now they are gone and only websocket.mojom remains while
we don't have the ownership of the file. Should we keep this directory,
or delete it and move websocket.mojom to somewhere?

https://codereview.chromium.org/2224713002/

da...@chromium.org

unread,
Aug 24, 2016, 1:15:05 PM8/24/16
to yhi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org
Thanks for reviewing!



https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp#newcode15
third_party/WebKit/Source/modules/websockets/WebSocketHandle.cpp:15:
#include "wtf/Functional.h"
On 2016/08/24 05:01:00, yhirano wrote:
> +wtf/text/WTFString.h

Done.


https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.h
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandle.h#newcode36
third_party/WebKit/Source/modules/websockets/WebSocketHandle.h:36:
#include "wtf/Vector.h"
On 2016/08/24 05:01:00, yhirano wrote:
> +wtf/Forward.h

Done.


https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
File
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(left):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h#oldcode63
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h:63:
virtual void didFail(WebSocketHandle* /* handle */, const WebString&
message) = 0;
On 2016/08/24 05:01:00, yhirano wrote:
> Can you leave a commented out parameter name if it is used in the
function
> comment? Ditto below.

Done.


https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
File
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h#newcode34
third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h:34:
#include "modules/websockets/WebSocketHandle.h"
On 2016/08/24 05:01:00, yhirano wrote:
> +wtf/Forward.h

Done.
On 2016/08/24 05:01:00, yhirano wrote:
> I originally created this directory in order to give us ownership of
the related
> files. Now they are gone and only websocket.mojom remains while we
don't have
> the ownership of the file. Should we keep this directory, or delete it
and move
> websocket.mojom to somewhere?

Good point. However, I'm not sure where else to move it. We will surely
end up with more similar cases as more and more public header files are
replaced with mojoms. Maybe then we will see what organizational pattern
should replace the current model?

https://codereview.chromium.org/2224713002/

yhi...@chromium.org

unread,
Aug 24, 2016, 8:28:34 PM8/24/16
to da...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 24, 2016, 11:59:37 PM8/24/16
to da...@chromium.org, yhi...@chromium.org, commi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 12:08:02 AM8/25/16
to da...@chromium.org, yhi...@chromium.org, commi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/245715)

https://codereview.chromium.org/2224713002/

da...@chromium.org

unread,
Aug 25, 2016, 11:59:16 AM8/25/16
to yhi...@chromium.org, tse...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org
+TBR=tsepez for IPC review. This is actually a no-op. I'm just moving the mojom
file from one folder to another.

https://codereview.chromium.org/2224713002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 12:00:03 PM8/25/16
to da...@chromium.org, yhi...@chromium.org, tse...@chromium.org, commi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 12:05:10 PM8/25/16
to da...@chromium.org, yhi...@chromium.org, tse...@chromium.org, commi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org
Committed patchset #12 (id:220001)

https://codereview.chromium.org/2224713002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 12:07:04 PM8/25/16
to da...@chromium.org, yhi...@chromium.org, tse...@chromium.org, commi...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org
Patchset 12 (id:??) landed as
https://crrev.com/b8df2228b7039e0326e6098e74547539fe9ef4e0
Cr-Commit-Position: refs/heads/master@{#414449}

https://codereview.chromium.org/2224713002/

joh...@chromium.org

unread,
Aug 25, 2016, 1:05:47 PM8/25/16
to da...@chromium.org, yhi...@chromium.org, tse...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org
This seems to have broken the Android Builder compile:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/Android%20Builder/builds/91243/steps/compile/logs/stdio
Linker complains about multiple definition of
blink::WebSocketHandle::OnAddChannelResponse and various others in
webkit_unit_tests

https://codereview.chromium.org/2224713002/

joh...@chromium.org

unread,
Aug 25, 2016, 1:11:28 PM8/25/16
to da...@chromium.org, yhi...@chromium.org, tse...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, nasko+c...@chromium.org, yzshen...@chromium.org, kinuko...@chromium.org, ben+...@chromium.org, yhiran...@chromium.org, j...@chromium.org, aba...@chromium.org, dglazko...@chromium.org, dari...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, loading...@chromium.org, har...@chromium.org, jap...@chromium.org, tyoshin...@chromium.org, a...@chromium.org, da...@chromium.org
On 2016/08/25 17:05:47, johnme wrote:
> This seems to have broken the Android Builder compile:
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/Android%20Builder/builds/91243/steps/compile/logs/stdio
> Linker complains about multiple definition of
> blink::WebSocketHandle::OnAddChannelResponse and various others in
> webkit_unit_tests

Reply all
Reply to author
Forward
0 new messages