Move WebSocketHandleImpl into Blink

Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org

Committed: https://crrev.com/b8df2228b7039e0326e6098e74547539fe9ef4e0
Cr-Commit-Position: refs/heads/master@{#414449}

[darin (slow to review), 2016-08-06 20:01:35]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 20:01:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 20:14:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 20:14:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-06 20:31:17]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 20:31:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 20:50:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 20:50:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-07 04:56:15]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-07 04:56:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-07 05:15:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-07 05:15:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-12 22:12:05]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 22:12:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 23:01:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 23:01:18]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[darin (slow to review), 2016-08-15 22:38:53]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 22:39:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 23:32:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 23:32:07]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[darin (slow to review), 2016-08-22 17:18:28]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 17:19:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darin (slow to review), 2016-08-22 17:28:32]

darin@chromium.org changed reviewers:
+ yhirano@chromium.org

[darin (slow to review), 2016-08-22 17:28:33]

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

https://codereview.chromium.org/2224713002/diff/180001/third_party/WebKit/Sou...
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.

[commit-bot: I haz the power, 2016-08-22 18:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 18:10:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[darin (slow to review), 2016-08-22 19:28:45]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 19:29:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 00:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 00:49:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[darin (slow to review), 2016-08-23 02:48:12]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 02:48:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 04:31:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 04:31:04]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-24 05:01:00]

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

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.h (right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(left):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/pub...
File third_party/WebKit/public/platform/modules/websockets/OWNERS (right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/websockets/OWNERS:2:
yhirano@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?

[darin (slow to review), 2016-08-24 17:15:05]

Thanks for reviewing!

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

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandle.h (right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(left):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleClient.h
(right):

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/Sou...
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.

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

https://codereview.chromium.org/2224713002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/websockets/OWNERS:2:
yhirano@chromium.org
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?

[darin (slow to review), 2016-08-24 17:28:12]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 17:28:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 19:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 19:07:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-25 00:28:33]

lgtm

[darin (slow to review), 2016-08-25 03:59:16]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2016-08-25 03:59:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 04:07:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 04:08:01]

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_presub...)

[darin (slow to review), 2016-08-25 15:58:44]

Description was changed from

==========
Move WebSocketHandleImpl into Blink
==========

to

==========
Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org
==========

[darin (slow to review), 2016-08-25 15:58:44]

darin@chromium.org changed reviewers:
+ tsepez@chromium.org

[darin (slow to review), 2016-08-25 15:59:15]

+TBR=tsepez for IPC review. This is actually a no-op. I'm just moving the mojom
file from one folder to another.

[darin (slow to review), 2016-08-25 15:59:45]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2016-08-25 16:00:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 16:05:08]

Description was changed from

==========
Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org
==========

to

==========
Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org
==========

[commit-bot: I haz the power, 2016-08-25 16:05:10]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-25 16:07:01]

Description was changed from

==========
Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org
==========

to

==========
Move WebSocketHandleImpl into Blink

TBR=tsepez@chromium.org

Committed: https://crrev.com/b8df2228b7039e0326e6098e74547539fe9ef4e0
Cr-Commit-Position: refs/heads/master@{#414449}
==========

[commit-bot: I haz the power, 2016-08-25 16:07:03]

Patchset 12 (id:??) landed as
https://crrev.com/b8df2228b7039e0326e6098e74547539fe9ef4e0
Cr-Commit-Position: refs/heads/master@{#414449}

[johnme, 2016-08-25 17:05:47]

This seems to have broken the Android Builder compile:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/Android%20Bui...
Linker complains about multiple definition of
blink::WebSocketHandle::OnAddChannelResponse and various others in
webkit_unit_tests

[johnme, 2016-08-25 17:11:28]

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%20Bui...
> Linker complains about multiple definition of
> blink::WebSocketHandle::OnAddChannelResponse and various others in
> webkit_unit_tests

Ah - I see you reverted this in https://codereview.chromium.org/2279763003.
Thanks!